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

split framework module #5

Merged
merged 9 commits into from
Apr 1, 2020
Merged

split framework module #5

merged 9 commits into from
Apr 1, 2020

Conversation

gmalkov
Copy link
Contributor

@gmalkov gmalkov commented Mar 31, 2020

into terra-cluster and terra-env

into terra-cluster and terra-env
identity-concentrator/outputs.tf Outdated Show resolved Hide resolved
identity-concentrator/variables.tf Show resolved Hide resolved
poc-service/variables.tf Show resolved Hide resolved
terra-env/variables.tf Show resolved Hide resolved
poc-service/outputs.tf Outdated Show resolved Hide resolved
poc-service/variables.tf Outdated Show resolved Hide resolved
poc-service/variables.tf Outdated Show resolved Hide resolved
terra-cluster/sa.tf Outdated Show resolved Hide resolved
terra-env/apps.tf~Stashed changes Outdated Show resolved Hide resolved
terra-env/outputs.tf Show resolved Hide resolved
Copy link
Contributor

@yonghaoy yonghaoy left a comment

Choose a reason for hiding this comment

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

LG! Thanks for doing this!
Left few questiosns and minor comment.

identity-concentrator/cloudsql.tf Outdated Show resolved Hide resolved
identity-concentrator/outputs.tf Show resolved Hide resolved
identity-concentrator/outputs.tf Show resolved Hide resolved
identity-concentrator/variables.tf Outdated Show resolved Hide resolved
terra-cluster/variables.tf Outdated Show resolved Hide resolved
@@ -0,0 +1,23 @@
module "poc_service" {
Copy link
Contributor

Choose a reason for hiding this comment

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

  • For new apps that might be developed over long periods... is the idea that developers maintain a long-lived branch of the terra-env module? Or would we encourage people to add enabled/disabled flags to the new app's module and stay integrated with master? Also, are we anticipating there's a linear version stream on master that is used to push changes from integration -> staging/pre-prod -> prod or will we try to keep them pegged to master and use vars to control rollout of new components instead?

  • I wish there was a way to flexibly compose environments with different sets of service modules together! In an ideal world, I could have a configuration file for an environment that specifies the modules I want it to include, the versions for each module, and any vars I want to override. Obviously, that is not how Terraform works 😅

Copy link
Contributor

@choover-broad choover-broad Apr 1, 2020

Choose a reason for hiding this comment

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

One more question - are we in general expecting to run a full copy of Terra in the new dev/integration environments? So it makes sense to include Cromwell resources there, and everything else eventually?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are all really good questions.

Is the idea that developers maintain a long-lived branch of the terra-env module? Or would we encourage people to add enabled/disabled flags to the new app's module and stay integrated with master?

IMO the latter seems cleaner to me. Especially since it looks like we will very soon have count and for_each available for modules, so we won't even need to have flags, we can just have booleans that toggle all the apps in the terra-env module, that can be set from per-environment tfvars files:
hashicorp/terraform#24461

Also, are we anticipating there's a linear version stream on master that is used to push changes from integration -> staging/pre-prod -> prod or will we try to keep them pegged to master and use vars to control rollout of new components instead?

I've been thinking about this. It's tricky because you can't have dynamic evaluation of the module source, so we can't have a variable that dictates a sub-module version. Some possible ways to roll out changes:

  • If we can find a clean way to specify a module version per workspace, I would really prefer the approach of a linear version stream of terra-env. One avenue that might be worth investigating is override files. As of now I don't see a clean way to do this.
  • If we can't find such a way, we can open a PR and keep it open until the change propagates all the way through the various environments, doing atlantis apply for each environment as we are ready for the changes to come in. After we do prod, merge the PR. This could potentially get messy if there are multiple such long-running PRs open.
  • Using tfvars files to roll out new components might currently be the cleanest way to go, especially when that change I linked above gets released.

I wish there was a way to flexibly compose environments with different sets of service modules together!

I think the change I linked will in fact make this possible to implement cleanly, with counts per module we can toggle apps per-environment via tfvars files for terra-env!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One more question - are we in general expecting to run a full copy of Terra in the new dev/integration environments? So it makes sense to include Cromwell resources there, and everything else eventually?

As I see it, we will only run a "full copy" of Terra in these environments when all of Terra is ported to k8s and whatever standard of k8s config/deployment tooling we settle on.

terra-env/outputs.tf Show resolved Hide resolved
@gmalkov gmalkov merged commit d1d94a0 into master Apr 1, 2020
@gmalkov gmalkov deleted the gm-framework-refactor branch April 1, 2020 19:58
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.

5 participants