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

Convert ingress-nginx app to use new app style #222 #224

Merged
merged 1 commit into from
Oct 13, 2020
Merged

Convert ingress-nginx app to use new app style #222 #224

merged 1 commit into from
Oct 13, 2020

Conversation

nitishkumar71
Copy link
Contributor

@nitishkumar71 nitishkumar71 commented Oct 12, 2020 •
edited by Waterdrips

Signed-off-by: Nitishkumar Singh nitishkumarsingh71@gmail.com

Description

Changes are made to convert existing ingress-nginx use the new app style.

Motivation and Context

How Has This Been Tested?

  1. made changes
  2. generate build using make build
  3. use new binary to install nginx-ingress app
  4. Deployed simple http services
  5. Deployed in default namespace and used kubectl port-forward svc/ingress-nginx-controller. Services were accessible from ingress
  6. Deployed in custom namespace and used kubectl port-forward svc/ingress-nginx-controller. Services were accessible from ingress

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have tested this on arm, or have added code to prevent deployment

Waterdrips
Waterdrips suggested changes Oct 13, 2020
View reviewed changes
Copy link
Contributor

@Waterdrips Waterdrips left a comment

Choose a reason for hiding this comment

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

Couple of small changes requested, Ill give this a test locally when those are done.

Thanks for taking a look at this :)

cmd/apps/nginx_app.go Show resolved Hide resolved
cmd/apps/nginx_app.go Show resolved Hide resolved
@Waterdrips
Copy link
Contributor

Great, can you squash the 2 commits into one?

git rebase -i origin/master
# then follow the instructions, probably squash one into the other

# when you are happy 
git push --force

@nitishkumar71
convert nginx app to use new app style alexellis#222
e41006a 
Signed-off-by: Nitishkumar Singh <nitishkumarsingh71@gmail.com>

included wait option and removed duplicated line

Signed-off-by: Nitishkumar Singh <nitishkumarsingh71@gmail.com>
@nitishkumar71
Copy link
Contributor Author

git push --force

Done

@Waterdrips
Copy link
Contributor

git push --force

Done

Great, ill have a look locally shortly. Thanks :)

Waterdrips
Waterdrips approved these changes Oct 13, 2020
View reviewed changes
Copy link
Contributor

@Waterdrips Waterdrips left a comment

Choose a reason for hiding this comment

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

Tested this locally with, defaults, namespace override and host mode. All worked a charm on AMD 64

Thanks :)

alexellis
alexellis approved these changes Oct 13, 2020
View reviewed changes
Copy link
Owner

@alexellis alexellis left a comment

Choose a reason for hiding this comment

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

Approved

@alexellis alexellis merged commit 479e2f1 into alexellis:master Oct 13, 2020
@alexellis alexellis changed the title convert nginx app to use new app style #222 Convert ingress-nginx app to use new app style #222 Oct 13, 2020
@nitishkumar71 nitishkumar71 deleted the nginx_new_helm_app_style branch March 16, 2024 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexellis alexellis alexellis approved these changes

@Waterdrips Waterdrips Waterdrips approved these changes

Assignees
No one assigned
Labels
new-contributor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Convert Nginx App to use "new" pkg/apps/helm_app style
3 participants
@nitishkumar71 @Waterdrips @alexellis