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

Initial Kustomize work #785

Merged
merged 1 commit into from
Nov 4, 2019
Merged

Initial Kustomize work #785

merged 1 commit into from
Nov 4, 2019

Conversation

tobbbles
Copy link
Contributor

Towards #424.

Here I provide a base bundle.yaml that I took from the example in the documentation. Ideally, I'd prefer to generate this with a script upon release to update the container image.

I've provided some base structural documentation of what envvars need to be patched from the kustomize manifest.

I'd love to hear some feedback and how we can continue on this. Thanks!

@codecov
Copy link

codecov bot commented Sep 23, 2019

Codecov Report

Merging #785 into master will increase coverage by 0.95%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #785      +/-   ##
==========================================
+ Coverage   71.83%   72.78%   +0.95%     
==========================================
  Files          65       63       -2     
  Lines        5213     4807     -406     
==========================================
- Hits         3745     3499     -246     
+ Misses       1183     1050     -133     
+ Partials      285      258      -27
Impacted Files Coverage Δ
server/events/models/models.go 74.6% <0%> (-0.4%) ⬇️
server/events/yaml/parser_validator.go 96.66% <0%> (-0.15%) ⬇️
server/events/comment_parser.go 95.2% <0%> (-0.08%) ⬇️
server/locks_controller.go 87.14% <0%> (ø) ⬆️
server/events/vcs/proxy.go 0% <0%> (ø) ⬆️
server/user_config.go 100% <0%> (ø) ⬆️
server/events/vcs/azuredevops_client.go
server/azuredevops_request_validator.go
cmd/server.go 79.42% <0%> (+0.65%) ⬆️
server/events/project_command_builder.go 83.01% <0%> (+0.89%) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 777956c...78d01f8. Read the comment docs.

Copy link
Member

@lkysow lkysow left a comment

Choose a reason for hiding this comment

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

Nice work!

Can you put the files inside a kustomize/ directory? Do they need to be at the root?

#### Required
```yaml
containers:
- name: atlantis
Copy link
Member

Choose a reason for hiding this comment

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

spacing is off here

bundle.yaml Outdated
fsGroup: 1000 # Atlantis group (1000) read/write access to volumes.
containers:
- name: atlantis
image: runatlantis/atlantis:v0.9.0
Copy link
Member

Choose a reason for hiding this comment

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

Can you follow the same pattern as the Kubernetes manifest deployment instructions and use

        image: runatlantis/atlantis:v<VERSION> # 1. Replace <VERSION> with the most recent release.

Then include this in your patch instructions? Or how do kustomize templates usually work? If that's not possible then add updating this file to the release instructions in CONTRIBUTING.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can - however from most examples I see, a lot of these bundles will be generated upon release and include the latest version in the bundle/kustomization file. It gives the user the a way to run the remote resource without any manifest issues. Sure, Atlantis won't run if they don't patch the environment variables - however that's the application failing here, rather than the actual Kubernetes manifest.

Let me know your thoughts on this.

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense. Can you add a bullet to our CONTRIBUTING.md file under "releases" to say update the kustomize/bundle.yaml file. You'll see we already say to update main.go.

@tobbbles
Copy link
Contributor Author

I believe the kustomization.yaml file has to be in the root of the project.

We could move the bundle into it's own directory

@lkysow
Copy link
Member

lkysow commented Oct 29, 2019

I believe the kustomization.yaml file has to be in the root of the project.

We could move the bundle into it's own directory

In that doc they use target="github.com/kubernetes-sigs/kustomize//examples/multibases/dev/?ref=v1.0.6" which doesn't have kustomize.yaml at its root so I think it's fine to move everything under a kustomize/ directory.

@lkysow lkysow added the waiting-on-response Waiting for a response from the user label Oct 29, 2019
@tobbbles tobbbles requested a review from lkysow November 1, 2019 07:59
@tobbbles
Copy link
Contributor Author

tobbbles commented Nov 1, 2019

Thanks for the feedback - updated the documentation accordingly along with the requested changes.

Copy link
Member

@lkysow lkysow left a comment

Choose a reason for hiding this comment

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

Great work!

@lkysow
Copy link
Member

lkysow commented Nov 1, 2019

Can you squash your commits and then we're ready to merge.

@tobbbles
Copy link
Contributor Author

tobbbles commented Nov 2, 2019

Thanks - squashed down for merging. 👍

@lkysow lkysow merged commit 663cfc6 into runatlantis:master Nov 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-response Waiting for a response from the user
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants