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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add deployment manifests as release artifacts #1092

Merged
merged 2 commits into from
Sep 21, 2023

Conversation

sjberman
Copy link
Contributor

@sjberman sjberman commented Sep 20, 2023

Problem: As a user, I want an easy way to install the NGF manifests. Currently, I have to clone the repository.

Solution: By including the manifests as part of a release, the repo no longer has to be cloned to install NGF. Note: the service definitions are not uploaded and therefore still require a clone if they are to be installed.

Testing: Created a release in my fork and verified that the artifacts existed. Successfully installed NGF using those artifacts.

Example release:
Screenshot 2023-09-20 at 2 03 45 PM

Closes #913

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

Problem: As a user, I want an easy way to install the NGF manifests. Currently, I have to clone the repository.

Solution: By including the manifests as part of a release, the repo no longer has to be cloned to install NGF. Note: the service definitions are not uploaded and therefore still require a clone if they are to be installed.
@sjberman sjberman requested a review from a team as a code owner September 20, 2023 20:01
@github-actions github-actions bot added the chore Pull requests for routine tasks label Sep 20, 2023
@bjee19
Copy link
Contributor

bjee19 commented Sep 20, 2023

Should this part of the AC from #913 be addressed?

Update the yaml installation documentation and include instructions on creating a service to work with NKG NGF.

@sjberman
Copy link
Contributor Author

@bjee19 That will have to be addressed after the release, as part of the release story.

@pleshakov
Copy link
Contributor

pleshakov commented Sep 21, 2023

@sjberman

Note: the service definitions are not uploaded and therefore still require a clone if they are to be installed.

Can we update the docs for that we can just run kubectl apply -f ... against a file in the Git repo under specific tag.
This way the repo doesn't have to be cloned.

or include them as part of the release too

Service is important - as it exposes the data plane to outside world

@pleshakov pleshakov mentioned this pull request Sep 21, 2023
6 tasks
@sjberman
Copy link
Contributor Author

@pleshakov We didn't want to include the Services for a specific reason which is escaping me...maybe the team can help me remember.

kubectl apply -f https://github.com/nginxinc/nginx-gateway-fabric/blob/main/deploy/manifests/service/nodeport.yaml does not work.

@ciarams87
Copy link
Member

ciarams87 commented Sep 21, 2023

@sjberman

@pleshakov We didn't want to include the Services for a specific reason which is escaping me...maybe the team can help me remember.

Services are environment specific, so I think that's why. Although I suppose we could publish each service manifest as a separate deployment artifact.

kubectl apply -f https://github.com/nginxinc/nginx-gateway-fabric/blob/main/deploy/manifests/service/nodeport.yaml does not work.

kubectl apply -f https://raw.githubusercontent.com/nginxinc/nginx-gateway-fabric/main/deploy/manifests/service/nodeport.yaml works

@sjberman
Copy link
Contributor Author

@ciarams87 Oh you're right, the second one works. I ran it incorrectly so it didn't work for me.

@sjberman
Copy link
Contributor Author

Do we expect to have more Service definitions in the future? I don't want to clutter our artifacts too much.

@pleshakov
Copy link
Contributor

pleshakov commented Sep 21, 2023

@sjberman

Do we expect to have more Service definitions in the future? I don't want to clutter our artifacts too much.

Those were chosen to cover most common cases. I don't think we will need more - those will be more specific cases

Also note: If/when we switch to the provisioner only, the provisioner will start creating load balancer service.

@sjberman
Copy link
Contributor Author

@pleshakov FWIW, I've set the AC in the release story to update the documentation for cloneless install.

Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

👍

@sjberman
Copy link
Contributor Author

@pleshakov Still think it's worth uploading Services as an artifact?

@pleshakov
Copy link
Contributor

@sjberman

@pleshakov Still think it's worth uploading Services as an artifact?

I see what's important is for the instructions to not require git clone (or download )- but how it is done is not as important in my view

@sjberman sjberman merged commit c98b2fa into nginxinc:main Sep 21, 2023
22 checks passed
@sjberman sjberman deleted the chore/manifest-artifacts branch September 21, 2023 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Pull requests for routine tasks
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Add deployment manifests as a release artifact
5 participants