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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

test: allow specifying custom nginx image for multus validation #12231

Merged
merged 1 commit into from
May 24, 2023
Merged

test: allow specifying custom nginx image for multus validation #12231

merged 1 commit into from
May 24, 2023

Conversation

iPraveenParihar
Copy link
Contributor

@iPraveenParihar iPraveenParihar commented May 13, 2023 •
edited

Description of your changes:

  • allow overriding the nginx server image for multus-validation-test-web-server from CLI using --nginx-image flag
  • default image will be nginxinc/nginx-unprivileged:stable-alpine

Which issue is resolved by this Pull Request:
Resolves #12175
CC @BlaineEXE

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
  • Skip Tests for Docs: If this is only a documentation change, add the label skip-ci on the PR.
  • Reviewed the developer guide on Submitting a Pull Request
  • Pending release notes updated with breaking and/or notable changes for the next minor release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

@iPraveenParihar iPraveenParihar marked this pull request as ready for review May 13, 2023 12:09
@iPraveenParihar iPraveenParihar force-pushed the custom-nginx-image-multus-test branch 2 times, most recently from 504da81 to 5fe6526 Compare May 15, 2023 14:07
@iPraveenParihar
Copy link
Contributor Author

Flags -

Screenshot from 2023-05-16 08-31-27

usage -
Screenshot from 2023-05-16 08-33-46

BlaineEXE
BlaineEXE requested changes May 18, 2023
View reviewed changes
Copy link
Member

@BlaineEXE BlaineEXE left a comment

Choose a reason for hiding this comment

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

Thanks for these changes! We're really glad to see interest in the tool we have just started releasing.

The changes are concise, and it catches most of the important corners where changes are needed. I'm requesting some changes largely for ensuring a clean codebase and a clear, succinct UI.

The more important functional changes are verifying the input value is non-empty, and ensuring the image is used for both the web server and clients. We decided to use the same image for both for simplicity, because the image is small, and because the Nginx image is already set up with a non-root user.

pkg/daemon/multus/validation.go Outdated Show resolved Hide resolved
pkg/daemon/multus/templates.go Outdated Show resolved Hide resolved
pkg/daemon/multus/nginx-pod.yaml Outdated Show resolved Hide resolved
cmd/rook/userfacing/multus/validation/validation.go Outdated Show resolved Hide resolved
cmd/rook/userfacing/multus/validation/validation.go Outdated Show resolved Hide resolved
pkg/daemon/multus/templates.go Outdated Show resolved Hide resolved
cmd/rook/userfacing/multus/validation/validation.go Outdated Show resolved Hide resolved
pkg/daemon/multus/templates.go Outdated Show resolved Hide resolved
pkg/daemon/multus/nginx-pod.yaml Outdated Show resolved Hide resolved
BlaineEXE
BlaineEXE requested changes May 19, 2023
View reviewed changes
Copy link
Member

@BlaineEXE BlaineEXE left a comment

Choose a reason for hiding this comment

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

Please also squash all commits into one, and include a short paragraph in the commit body explaining what the commit does.

@BlaineEXE
Copy link
Member

Also, now that #12211 is merged, the image will also have to get set for the image-pull daemonset.

@mergify
Copy link

mergify bot commented May 19, 2023

This pull request has merge conflicts that must be resolved before it can be merged. @iPraveenParihar please rebase it. https://rook.io/docs/rook/latest/Contributing/development-flow/#updating-your-fork

test: allow specifying custom nginx image for multus validation
78d6528 
Allow overriding the nginx server image used for the web server and clients from CLI with --nginx-image flag.
Set default image in flag as 'nginxinc/nginx-unprivileged:stable-alpine'

Signed-off-by: iPraveenParihar <praveenparihar68@gmail.com>
@BlaineEXE
Copy link
Member

Looks great! Thanks, @iPraveenParihar !

@BlaineEXE BlaineEXE added backport-release-1.11 test unit or integration testing labels May 24, 2023
@BlaineEXE BlaineEXE merged commit eb54bf5 into rook:master May 24, 2023
46 of 50 checks passed
@mergify mergify bot mentioned this pull request May 24, 2023
Merged
BlaineEXE added a commit that referenced this pull request May 24, 2023
@BlaineEXE
Merge pull request #12281 from rook/mergify/bp/release-1.11/pr-12231
f16bc44 
test: allow specifying custom nginx image for multus validation (backport #12231)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BlaineEXE BlaineEXE BlaineEXE approved these changes

Assignees
No one assigned
Labels
backport-release-1.11 test unit or integration testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow specifying custom nginx image for multus validation
2 participants
@iPraveenParihar @BlaineEXE