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

feat: use SNYK_CFG for IaC OCI registry env vars [CFG-1165] #2336

Merged
merged 2 commits into from
Nov 8, 2021

Conversation

teodora-sandu
Copy link
Contributor

@teodora-sandu teodora-sandu commented Nov 2, 2021

What does this PR do?

This PR changes the IaC OCI registry env vars to be set via snyk config set by using the SNYK_CFG_ prefix.

Where should the reviewer start?

The code change is pretty small and straightforward, but there will be other places we'll need to change these.
For now, the tests work without changing the CircleCI environment variables because we read them and then pass them with SNYK_CFG_ prefixed to them in the acceptance tests. I will need to update them in the public docs, 1Password, and the contract tests in https://github.com/snyk/snyk-iac-rules

How should this be manually tested?

  1. Run npm run build
  2. Run the CLI with no configs: snyk-dev iac test test/fixtures/iac/terraform/sg_open_ssh.tf
  3. Use the details in the IaC Tests - OCI Registries Credentials vault to set the following:
snyk-dev config set oci-registry-url=
snyk-dev config set oci-registry-username=
snyk-dev config set oci-registry-password=
  1. Run the CLI with the configs: snyk-dev iac test test/fixtures/iac/terraform/sg_open_ssh.tf

Any background context you want to provide?

We agreed on this in this Slack thread.

What are the relevant tickets?

https://snyksec.atlassian.net/browse/CFG-1165

Screenshots

Step 2:
Screenshot 2021-11-02 at 16 32 37

Step 4:
Screenshot 2021-11-02 at 16 35 13

Help docs:
Screenshot 2021-11-08 at 11 05 29
Screenshot 2021-11-08 at 11 10 16
Screenshot 2021-11-08 at 11 10 21
Screenshot 2021-11-08 at 11 12 03

@teodora-sandu teodora-sandu requested review from a team and craigfurman November 2, 2021 16:42
@teodora-sandu teodora-sandu changed the title feat: use SNYK_CFG for IaC OCI registry env vars feat: use SNYK_CFG for IaC OCI registry env vars [CFG-1165] Nov 2, 2021
@teodora-sandu teodora-sandu requested review from ipapast and removed request for craigfurman November 2, 2021 16:42
@@ -92,22 +92,22 @@ describe('custom rules pull from a remote OCI registry', () => {
test.each(cases)(
'given %p as a registry and correct credentials, it returns a success exit code',
async (
OCI_REGISTRY_NAME,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ipapast We can get away without changing the env vars in CircleCI (and the same for snyk-iac-rules), but I'm wondering if you think it's confusing?

Copy link
Contributor

@ipapast ipapast Nov 5, 2021

Choose a reason for hiding this comment

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

I think let's keep them consistent, it's not a big deal to change them in CircleCI. (we also might not remember this in a few months time :) )
+++ update in 1password

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the variables in the code but will change them in CircleCI when I'm close to merging, so we don't break existing pipelines

Copy link
Contributor

Choose a reason for hiding this comment

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

we should go with expand and contract; have both the new and old values on the first merge, remove after the merge. just in case someone's releasing at the same time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I just realised that the env vars in CircleCI have nothing to do with the SNYK_CFG env vars. They are strictly configured for the tests, so we can then pass them to the SNYK_CFG env vars. So I will keep them as they are

Copy link
Contributor

Choose a reason for hiding this comment

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

AH!! yes, you are very right, I explicitly named them with the registry name inside (docker, etc) for this reason, I should have remembered it 😆

Copy link
Contributor

@ipapast ipapast 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 with set, unset, clear and it works great.
We had a chat on Slack with Teo about generating docs to show the usage of these env vars too.
I'm approving as it works great, some docs will be added soon

@teodora-sandu teodora-sandu requested a review from a team as a code owner November 5, 2021 13:48
@teodora-sandu teodora-sandu force-pushed the feat/use-snyk-cfg-oci-registry branch 2 times, most recently from 4279330 to 2f92ad3 Compare November 8, 2021 10:00
@teodora-sandu teodora-sandu force-pushed the feat/use-snyk-cfg-oci-registry branch 5 times, most recently from 2d1c68e to 828972e Compare November 8, 2021 11:12
help/commands-docs/_ENVIRONMENT.md Outdated Show resolved Hide resolved
help/commands-txt/snyk-auth.txt Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants