[stable/atlantis] Add `--default-tf-version=` and `--allow-fork-prs` flag #13299

andyyaldoo · 2019-04-26T03:24:20Z

What this PR does / why we need it:

Allow users to specify default terraform version via --default-tf-version flag
Allow users to specify whether they want to allow --allow-fork-prs flag

Which issue this PR fixes

Special notes for your reviewer:

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

DCO signed
Chart Version bumped
Variables are documented in the README.md

Signed-off-by: Andy Aldo Dharmawan <andyaldodharmawan@gmail.com>

k8s-ci-robot · 2019-04-26T03:24:34Z

Hi @adharmawan. Thanks for your PR.

I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Signed-off-by: Andy Aldo Dharmawan <andyaldodharmawan@gmail.com>

stable/atlantis/README.md

Fix a mismatch between default values in documentation and in `values.yaml` Signed-off-by: Andy Aldo Dharmawan <andyaldodharmawan@gmail.com>

jeff-knurek · 2019-04-26T08:28:07Z

/lgtm

k8s-ci-robot · 2019-04-26T08:28:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adharmawan, jeff-knurek

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~stable/atlantis/OWNERS~~ [jeff-knurek]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

lkysow · 2019-04-26T12:13:24Z

stable/atlantis/templates/statefulset.yaml

@@ -74,6 +74,12 @@ spec:
          {{- end}}
          args:
            - server
+          {{- if .Values.allowForkPRs }}


@jeff-knurek I thought we were using environment variables for all the flags?

🤦‍♂️ yeah, that aligns with the convention - wasn't thinking

I'll submit the fix, but it'll be about a week before I can get to it

I can do it tomorrow. Sorry, didn’t know about this.

I can do it tomorrow. Sorry, didn’t know about this.

thanks 👍

my thoughts:

make it a major version bump, cause anyone who might have tried using it in the meantime would break when these values get removed again

maybe add a section to the readme for using environment so people (including me) are less likely to try to make this kind of PR in the future :) - ie, link to: https://www.runatlantis.io/docs/server-configuration.html#environment-variables

So just to be clear, I think we should keep the Values but instead of applying them via cli flags, apply them via environment variables like:

{{- if .Values.allowForkPRs }} - name: ATLANTIS_ALLOW_FORK_PRS value: {{ .Values.allowForkPRs }} {{- if .Values.defaultTFVersion }} - name: ATLANTIS_DEFAULT_TF_VERSION value: {{ .Values.defaultTFVersion }} {{- end }}

So that wouldn't require a major bump.

Also I know it's basically the same thing but at this point we may as well keep it consistent.

Yup, that’s what I have in mind (keeping the Values)

hmmm, how will it work if someone already adds ATLANTIS_ALLOW_FORK_PRS to the environment value?
that would create two env vars with the same key?
users' might then be stuck with this issue: kubernetes/kubernetes#58477

Opened #13321. I'm not sure if I can add you guys as reviewer on that PR after it is created @jeff-knurek @lkysow

hmmm, how will it work if someone already adds ATLANTIS_ALLOW_FORK_PRS to the environment value?
that would create two env vars with the same key?
users' might then be stuck with this issue: kubernetes/kubernetes#58477

Yeah, that might happen. Although, may I know what's the purpose of the environment env var here. Why not be explicit and set one env var for each flag?

what's the purpose of the environment env var here

the idea is that the chart isn't tightly coupled with the application. as in if a new version of the app adds/removes env value option, then the chart needs to be updated immediately as well. it also adds for the flexibility of deploying different versions of the app that use different env values

it seems there's already now 10 values that explicitly set the env var. I know I've run into that bug of duplicate env vars, and it's quite frustrating when you don't know about it, so maybe now that more env values are added it makes sense to make all options explicit??

* spinnaker-additional-configmaps: (158 commits) [stable/spinnaker] Bump chart version [stable/spinnaker] Allow to use existing additionalConfigMaps objects [stable/instana-agent] Add instana-agent chart to stable (helm#12799) [stable/spring-cloud-data-flow] apiGroup extension does not have permissions over Jobs (helm#12174) Fluentd - Add option to add environment variables from secrets (helm#12565) Fluentd - Allow ingress path to be configurable (helm#12561) [stable/openebs]: update NDM image tag to 0.3.5 (helm#13282) stable/phabricator: update to 2019.16.0 (helm#13307) [stable/jenkins] Add support for idleMinutes and serviceAccount (helm#13263) [stable/gocd] Bump up k8 elastic agent to latest and bump up GoCD to v19.3.0 (helm#13301) [stable/atlantis] Add `--default-tf-version=` and `--allow-fork-prs` flag (helm#13299) stackdriver-exporter: allow google service account (helm#13214) SC-4435 Do not start the container if particular token is not provided (helm#13304) [stable/spring-cloud-data-flow] Update to new SCDF version 2.0.2 (helm#12951) allow to set COCKROACH_ENGINE_MAX_SYNC_DURATION (helm#13244) Use same JENKINS_URL no matter if slaves use different namespace (helm#12564) stable/concourse: separate worker, web deployments (helm#12920) [ci] Upgrade to chart-testing v2.3.3 (helm#13294) fixes incompatibility with 1.11 (helm#13261) Detect current network and netmask (helm#13250) ... # Conflicts: # stable/spinnaker/Chart.yaml

…flag (helm#13299) * Allow to pass `allow-fork-prs` to atlantis server Signed-off-by: Andy Aldo Dharmawan <andyaldodharmawan@gmail.com> * Allow passing `--default-tf-version=` flag when starting atlantis server Signed-off-by: Andy Aldo Dharmawan <andyaldodharmawan@gmail.com> * Update README.md Signed-off-by: Andy Aldo Dharmawan <andyaldodharmawan@gmail.com> * Set allowForkPRs property default to false Signed-off-by: Andy Aldo Dharmawan <andyaldodharmawan@gmail.com> * Bump chart version Signed-off-by: Andy Aldo Dharmawan <andyaldodharmawan@gmail.com> * Add extra `#` for comments in values.yaml Signed-off-by: Andy Aldo Dharmawan <andyaldodharmawan@gmail.com> * Update comment in values.yaml Signed-off-by: Andy Aldo Dharmawan <andyaldodharmawan@gmail.com> * Update README.md Fix a mismatch between default values in documentation and in `values.yaml` Signed-off-by: Andy Aldo Dharmawan <andyaldodharmawan@gmail.com>

Allow to pass allow-fork-prs to atlantis server

185b80c

Signed-off-by: Andy Aldo Dharmawan <andyaldodharmawan@gmail.com>

k8s-ci-robot requested review from jeff-knurek and lkysow April 26, 2019 03:24

helm-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 26, 2019

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 26, 2019

andyyaldoo added 4 commits April 26, 2019 11:25

Allow passing --default-tf-version= flag when starting atlantis server

9ff26ae

Signed-off-by: Andy Aldo Dharmawan <andyaldodharmawan@gmail.com>

Update README.md

de2bc28

Signed-off-by: Andy Aldo Dharmawan <andyaldodharmawan@gmail.com>

Set allowForkPRs property default to false

1d12766

Signed-off-by: Andy Aldo Dharmawan <andyaldodharmawan@gmail.com>

Bump chart version

a385ae4

Signed-off-by: Andy Aldo Dharmawan <andyaldodharmawan@gmail.com>

helm-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Apr 26, 2019

andyyaldoo changed the title ~~Add --default-tf-version= and --allow-fork-prs options~~ [stable/atlantis] Add --default-tf-version= and --allow-fork-prs options Apr 26, 2019

Add extra # for comments in values.yaml

c94ca7b

Signed-off-by: Andy Aldo Dharmawan <andyaldodharmawan@gmail.com>

andyyaldoo changed the title ~~[stable/atlantis] Add --default-tf-version= and --allow-fork-prs options~~ [stable/atlantis] Add --default-tf-version= and --allow-fork-prs flag Apr 26, 2019

Update comment in values.yaml

1f040e6

Signed-off-by: Andy Aldo Dharmawan <andyaldodharmawan@gmail.com>

jeff-knurek reviewed Apr 26, 2019

View reviewed changes

stable/atlantis/README.md Outdated Show resolved Hide resolved

Update README.md

984175d

Fix a mismatch between default values in documentation and in `values.yaml` Signed-off-by: Andy Aldo Dharmawan <andyaldodharmawan@gmail.com>

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 26, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 26, 2019

k8s-ci-robot merged commit fe16482 into helm:master Apr 26, 2019

andyyaldoo deleted the patch branch April 26, 2019 08:42

lkysow reviewed Apr 26, 2019

View reviewed changes

andyyaldoo mentioned this pull request Apr 26, 2019

[stable/atlantis] Pass server configurations via environment variable #13321

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[stable/atlantis] Add `--default-tf-version=` and `--allow-fork-prs` flag #13299

[stable/atlantis] Add `--default-tf-version=` and `--allow-fork-prs` flag #13299

andyyaldoo commented Apr 26, 2019

k8s-ci-robot commented Apr 26, 2019

jeff-knurek commented Apr 26, 2019

k8s-ci-robot commented Apr 26, 2019

lkysow Apr 26, 2019

jeff-knurek Apr 26, 2019

andyyaldoo Apr 26, 2019

jeff-knurek Apr 26, 2019

lkysow Apr 26, 2019

andyyaldoo Apr 26, 2019 •

edited

Loading

jeff-knurek Apr 26, 2019

andyyaldoo Apr 26, 2019 •

edited

Loading

andyyaldoo Apr 26, 2019

jeff-knurek May 2, 2019

[stable/atlantis] Add --default-tf-version= and --allow-fork-prs flag #13299

[stable/atlantis] Add --default-tf-version= and --allow-fork-prs flag #13299

Conversation

andyyaldoo commented Apr 26, 2019

What this PR does / why we need it:

Which issue this PR fixes

Special notes for your reviewer:

Checklist

k8s-ci-robot commented Apr 26, 2019

jeff-knurek commented Apr 26, 2019

k8s-ci-robot commented Apr 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyyaldoo Apr 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyyaldoo Apr 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[stable/atlantis] Add `--default-tf-version=` and `--allow-fork-prs` flag #13299

[stable/atlantis] Add `--default-tf-version=` and `--allow-fork-prs` flag #13299

andyyaldoo Apr 26, 2019 •

edited

Loading

andyyaldoo Apr 26, 2019 •

edited

Loading