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

Default values for variables removed #539

Closed
wants to merge 0 commits into from
Closed

Default values for variables removed #539

wants to merge 0 commits into from

Conversation

marifse
Copy link
Contributor

@marifse marifse commented Feb 9, 2024

Description

Release required?

  • No release
  • Patch release (x.x.X)
  • Minor release (x.X.x)
  • Major release (X.x.x)
Release notes content

Run the pipeline

If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.

Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:

/run pipeline

Checklist for reviewers

  • If relevant, a test for the change is included or updated with this PR.
  • If relevant, documentation for the change is included or updated with this PR.

For mergers

  • Use a conventional commit message to set the release level. Follow the guidelines.
  • Include information that users need to know about the PR in the commit message. The commit message becomes part of the GitHub release notes.
  • Use the Squash and merge option.

@marifse
Copy link
Contributor Author

marifse commented Feb 9, 2024

@ocofaigh can you please check and verify.

@ocofaigh ocofaigh requested a review from SirSpidey February 13, 2024 12:29
Copy link
Member

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

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

@marifse I left one comment. Also adding @SirSpidey as reviewer for the wording you are adding

examples/complete/version.tf Outdated Show resolved Hide resolved
Copy link
Contributor

@SirSpidey SirSpidey left a comment

Choose a reason for hiding this comment

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

small editorial suggestions

examples/basic/README.md Outdated Show resolved Hide resolved
examples/basic/README.md Outdated Show resolved Hide resolved
@marifse
Copy link
Contributor Author

marifse commented Feb 19, 2024

@marifse I left one comment. Also adding @SirSpidey as reviewer for the wording you are adding

@ocofaigh can you please check now, requested fixes have been addressed.

SirSpidey
SirSpidey previously approved these changes Feb 23, 2024
Copy link
Contributor

@SirSpidey SirSpidey left a comment

Choose a reason for hiding this comment

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

👍

Copy link

@shemau shemau left a comment

Choose a reason for hiding this comment

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

Completing the description and referencing any issues might have helped understand. I did look for kms default issues and found none, but that may have been my search.

examples/complete/variables.tf Outdated Show resolved Hide resolved
@ocofaigh
Copy link
Member

ocofaigh commented Mar 3, 2024

/run pipeline

Copy link
Member

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

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

region and prefix values need to be added as TF_VARs in cra-config.yaml now that they are required vars in the example being scanned.

@marifse
Copy link
Contributor Author

marifse commented Mar 10, 2024

region and prefix values need to be added as TF_VARs in cra-config.yaml now that they are required vars in the example being scanned.

@ocofaigh values for region and prefix in cra-config.yml have been added.

cra-config.yaml Outdated
@@ -4,6 +4,8 @@ CRA_TARGETS:
- CRA_TARGET: "examples/complete" # Target directory for CRA scan. If not provided, the CRA Scan will not be run.
CRA_IGNORE_RULES_FILE: "cra-tf-validate-ignore-rules.json" # CRA Ignore file to use. If not provided, it checks the repo root directory for `cra-tf-validate-ignore-rules.json`
PROFILE_ID: "0e6e7b5a-817d-4344-ab6f-e5d7a9c49520" # SCC profile ID (currently set to the FSCloud 1.4.0 profile).
TF_VAR_regions: "us-south"
TF_VAR_prefix: "complete-kms"
Copy link
Member

Choose a reason for hiding this comment

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

these are not in correct place in yaml. They need to be under CRA_ENVIRONMENT_VARIABLES. See the commented out code below this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ocofaigh fixed.

@ocofaigh
Copy link
Member

/run pipeline

Copy link
Member

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

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

@marifse Looks like your PR contains an old version of the common-dev-assets git submodule, and so pipeline is failing. I suggest you don't commit updates to common-dev-assets to avoid this issue. Please follow the steps below:

Detected common-dev-assets git submodule commit ID is older than the one in primary branch. To fix:
  1. Record the current state of the working directory:
      i. git stash
  2. Ensure that your local primary branch is up to date:
      i. git checkout <master / main>
      ii. git pull origin <master / main>
      iii. git submodule update --rebase
  3. Make sure your dev branch is rebased with remote primary branch:
      i. git checkout <dev-branch>
      ii. git stash pop
      iii. git pull origin <master / main>
  4. Run the following command to sync the git submodule with primary branch:
      i. git submodule update --rebase

@marifse marifse closed this Mar 17, 2024
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.

4 participants