Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: okteto test to use always root user #4336

Merged
merged 5 commits into from
Jun 18, 2024
Merged

fix: okteto test to use always root user #4336

merged 5 commits into from
Jun 18, 2024

Conversation

jLopezbarb
Copy link
Contributor

@jLopezbarb jLopezbarb commented Jun 13, 2024

Proposed changes

Fixes DEV-423

If you set an image on a test that doesn't have root access on okteto test command, now okteto is able to run the command without an error

How to validate

  1. Create the following Dockerfile:
FROM ubuntu:latest

WORKDIR /app
COPY . .

RUN chown -R 1000:1000 /app
USER 1000
  1. Create the following okteto manifest
build:
  tests:
    context: .
test:
  hello:
    image: $OKTETO_BUILD_TESTS_IMAGE
    commands:
    - echo hello
  1. Run okteto test

CLI Quality Reminders 馃敡

For both authors and reviewers:

  • Scrutinize for potential regressions
  • Ensure key automated tests are in place
  • Build the CLI and test using the validation steps
  • Assess Developer Experience impact (log messages, performances, etc)
  • If too broad, consider breaking into smaller PRs
  • Adhere to our code style and code review guidelines

@jLopezbarb jLopezbarb requested a review from a team as a code owner June 13, 2024 11:42
Copy link

codecov bot commented Jun 13, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 43.28%. Comparing base (a809252) to head (2d186a8).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4336   +/-   ##
=======================================
  Coverage   43.28%   43.28%           
=======================================
  Files         375      375           
  Lines       30241    30243    +2     
=======================================
+ Hits        13090    13091    +1     
- Misses      16011    16012    +1     
  Partials     1140     1140           

Copy link
Contributor

@maroshii maroshii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is see we are only setting it to true in the go tests not in the command, am I missing something?

Signed-off-by: Javier Lopez <[email protected]>
@jLopezbarb
Copy link
Contributor Author

Is see we are only setting it to true in the go tests not in the command, am I missing something?

yup, when I divided the PR in two I didn't add the change to make it work on test 馃う

Copy link
Member

@ifbyol ifbyol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add an integration tests for this scenario? It doesn't have to be in this PR

@jLopezbarb jLopezbarb added the run-e2e When used on a PR run windows & unix e2e label Jun 18, 2024
@jLopezbarb jLopezbarb merged commit dac0caa into master Jun 18, 2024
18 of 19 checks passed
@jLopezbarb jLopezbarb deleted the jlo/dev-423 branch June 18, 2024 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release/bug-fix run-e2e When used on a PR run windows & unix e2e
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants