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

Update Org onboarding to honor include/exclude #39

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

ravinadhruve10
Copy link
Contributor

@ravinadhruve10 ravinadhruve10 commented Feb 27, 2025

Change summary:

  • For Foundational onboarding, update the stackset deployment targets to honor the various include/exclude ous/accounts configuration input(s) provided by the user.
  • Added a section in README.
  • Added the same for all feature & integration onboarding installs.
  • Updated the provider version, variables and READMEs for all feature & integration modules.

Change summary:
----------------
- For Foundational onboarding, update the stackset deployment targets
  to honor the various include/exclude ous/accounts configuration
  input(s) provided by the user.
- Added a section in README.
Copy link
Contributor

@jose-pablo-camacho jose-pablo-camacho left a comment

Choose a reason for hiding this comment

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

Overall I like the approach, thanks for all this work!

Left a couple of comments - one thing that we can do is. Since we know we won't have custom include/exclude params for each feature. Can we pass the list of deployment_targets and deployment_accounts passed from the onboarding module as output to variables within the rest of the modules? This way we avoid logic repeated in each module and decrease maintenance overload as well as possible errors when adding/fixing something.

@ravinadhruve10
Copy link
Contributor Author

Overall I like the approach, thanks for all this work!

Left a couple of comments - one thing that we can do is. Since we know we won't have custom include/exclude params for each feature. Can we pass the list of deployment_targets and deployment_accounts passed from the onboarding module as output to variables within the rest of the modules? This way we avoid logic repeated in each module and decrease maintenance overload as well as possible errors when adding/fixing something.

So I had thought about that earlier to pass on the deployment_targets and accounts. However if we do, we will end up exposing it in TF snippets which are user facing. It could get confusing for the user to have both include/exclude inputs & deployment targets as inputs, which is why I didn't think it would be the best idea.

Another thing I looked at is having a common locals.tf file across all modules. However it isn't directly possible and will require refactoring our module folder structure. Open to other ideas though.

@jose-pablo-camacho
Copy link
Contributor

Overall I like the approach, thanks for all this work!
Left a couple of comments - one thing that we can do is. Since we know we won't have custom include/exclude params for each feature. Can we pass the list of deployment_targets and deployment_accounts passed from the onboarding module as output to variables within the rest of the modules? This way we avoid logic repeated in each module and decrease maintenance overload as well as possible errors when adding/fixing something.

So I had thought about that earlier to pass on the deployment_targets and accounts. However if we do, we will end up exposing it in TF snippets which are user facing. It could get confusing for the user to have both include/exclude inputs & deployment targets as inputs, which is why I didn't think it would be the best idea.

Another thing I looked at is having a common locals.tf file across all modules. However it isn't directly possible and will require refactoring our module folder structure. Open to other ideas though.

@ravinadhruve10 - I got this, however IMO I would say it's better to have two variables passed to the other modules rather than replicating the logic in the modules since this is also needed for other features than maintaining same code across all features, what do you think?

I know we already pass some and expose important data through the TF snippet from the onboarding module to others like sysdig_secure_account_id = module.onboarding.sysdig_secure_account_id so that's why I thought it's not a bad idea to do this and we haven't had customers complain about it AFAIK.

What do you think? I personally prefer this so we have one source and relay the info to the other modules?

I agree having a shared local file would be great but I don't think it's in our scope for now. So my approach would be to pass it from onboarding modules to others and maybe if we get to refactor this we can setup this through the shared local file. I don't know if my point changed your way of seeing this, we can always chat offline

@ravinadhruve10 ravinadhruve10 marked this pull request as ready for review March 7, 2025 07:59
@ravinadhruve10 ravinadhruve10 requested review from a team as code owners March 7, 2025 07:59
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