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

AJ-1074 - Have WDS manage certain config variables on its own vs getting them from Leo #364

Merged
merged 35 commits into from
Oct 30, 2023

Conversation

yuliadub
Copy link
Contributor

I choose to use Spring Profiles since it is built in functionality that provides easy configuration per profile. The main change that we will need in Leo is for Leo to provide the profile name using the following env variable "spring_profiles_active". Based on that value WDS will first load the default application.yaml and then the specific profile (specific profile will overwrite any values application already defined, for example in local profile I am setting workspace_id)

I created 4 new profiles:

  • local (should be nice since now we only need to set 1 env variable to run locally, this profile will care of the rest)
  • dev
  • prod
  • bee

Will need to make change in Leo to exclude all the values moved to profile and instead set the spring_profiles_active for WDS and provide a BEE name when one is present.

Reminder:

PRs merged into main will not automatically generate a PR in https://github.com/broadinstitute/terra-helmfile to update the WDS image deployed to kubernetes - this action will need to be triggered manually by running the following github action: https://github.com/DataBiosphere/terra-workspace-data-service/actions/workflows/tag.yml. Dont forget to provide a Jira Id when triggering the manual action, if no Jira ID is provided the action will not fully succeed.

After you manually trigger the github action (and it completes with no errors), you must go to the terra-helmfile repo and verify that this generated a PR that merged successfully.

The terra-helmfile PR merge will then generate a PR in leonardo. This will automerge if all tests pass, but if jenkins tests fail it will not; be sure to watch it to ensure it merges. To trigger jenkins retest simply comment on PR with "jenkins retest".

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@jladieu
Copy link
Contributor

jladieu commented Oct 12, 2023

This is gonna be so great! Thanks for working on this. I like the changes in general but agree with @calypsomatic's observation that the TDR URL might need to be updated. Approving under the assumption that you'll work that out :)

@davidangb davidangb self-requested a review October 12, 2023 14:47
Copy link
Collaborator

@davidangb davidangb left a comment

Choose a reason for hiding this comment

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

A few comments inline - overall I like this approach and am so glad to see it happening.

We should review everything Leo sends and see if anything else can move over - for instance, the instrumentationEnabled value.

A kinda pedantic note: I don't think that Leo should know anything about WDS's Spring profiles. Leo should just send an "env" specification of prod, bee, dev, etc. …. then it's up to WDS and WDS's Helm chart to translate the "env" into a profile. This gives use more freedom in the future to interpret Leo's "env" in different ways.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@yuliadub
Copy link
Contributor Author

yuliadub commented Oct 12, 2023

A few comments inline - overall I like this approach and am so glad to see it happening.

We should review everything Leo sends and see if anything else can move over - for instance, the instrumentationEnabled value.

A kinda pedantic note: I don't think that Leo should know anything about WDS's Spring profiles. Leo should just send an "env" specification of prod, bee, dev, etc. …. then it's up to WDS and WDS's Helm chart to translate the "env" into a profile. This gives use more freedom in the future to interpret Leo's "env" in different ways.

do we actually use instrumentationEnabled? I cant seem to find it referenced anywhere in terra-helm.

And the million dollar question, is it time to remove the local pod postgres configuration?

@yuliadub yuliadub marked this pull request as ready for review October 20, 2023 20:45
README.md Show resolved Hide resolved
Copy link
Collaborator

@davidangb davidangb left a comment

Choose a reason for hiding this comment

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

👍 for syntax.

Before you merge - what is the rollout plan for this? In other words, what is the order of operations for merging PRs in Leo, terra-helmfile, WDS and releasing those to production? We need to make sure that we don't break older versions of WDS (see also: what's our plan for updating existing WDS deployments in prod), or break the app-upgrade process.

Copy link
Collaborator

@davidangb davidangb left a comment

Choose a reason for hiding this comment

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

A few more comments inline!

@davidangb davidangb self-requested a review October 24, 2023 21:16
README.md Outdated Show resolved Hide resolved
test.txt Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Oct 30, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@yuliadub yuliadub merged commit 211d970 into main Oct 30, 2023
12 checks passed
@yuliadub yuliadub deleted the aj-1074-wds-config branch October 30, 2023 16:15
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.

6 participants