Skip to content

[LG-6146] Prepare for removing ERB from service_providers.yml#7742

Merged
orenyk merged 1 commit intomainfrom
oyk-env-specific-yaml
Feb 8, 2023
Merged

[LG-6146] Prepare for removing ERB from service_providers.yml#7742
orenyk merged 1 commit intomainfrom
oyk-env-specific-yaml

Conversation

@orenyk
Copy link
Contributor

@orenyk orenyk commented Feb 1, 2023

Resolves LG-6146

Precursor to #7764 to avoid breaking things, should be safe to merge/deploy before the YAML files change.

@orenyk orenyk force-pushed the oyk-env-specific-yaml branch 2 times, most recently from 1baeae9 to ea854c2 Compare February 2, 2023 04:59
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

@julialeague
Copy link
Contributor

julialeague commented Feb 2, 2023

question: Can this be tested in a deployed environment (like ursula) before merging? Maybe it's not that important to test before merging because the worst that would happen is it would temporarily break our deployments and not the actual running instance?

@orenyk
Copy link
Contributor Author

orenyk commented Feb 3, 2023

@julialeague Unfortunately I don't think so - we'll have to coordinate merges since the file is used everywhere. With that said, I think we could probably make this a bit safer by splitting this up into two changes - first just adding the gsub for the %{env} replacement, which will do absolutely nothing with the current file, and then when we merge in the changes in identity-idp-config we can remove the ERB references (but they won't do anything bad since the file will not contain any ERB.

@zachmargolis does that seem right to you as a safer sequence of events? Thanks for the inspiration @julialeague!

@zachmargolis
Copy link
Contributor

question: Can this be tested in a deployed environment (like ursula) before merging? Maybe it's not that important to test before merging because the worst that would happen is it would temporarily break our deployments and not the actual running instance?

I think it is possible to test this before merging to main in a deployed env. If we patch this line to clone a branch of identity-idp-config instead, and then push the branch to say, team ursula env, it would be a pretty good way to test:

cmd = ['git', 'clone', '--depth', '1', '--branch', 'main', private_git_repo_url, checkout_dir]

@orenyk orenyk force-pushed the oyk-env-specific-yaml branch from 5dbeeb3 to b5bafe8 Compare February 6, 2023 20:55
@orenyk
Copy link
Contributor Author

orenyk commented Feb 7, 2023

I tried testing this in my personal sandbox but something strange is happening during seeding - I'm getting a validation error on a duplicate abbreviation when seeding agencies, which is weird since I recently nuked and re-provisioned my personal sandbox a few weeks ago and the agencies.yml file hasn't been touched in almost 8 months.

I'm pretty sure this PR should be safe to merge (it only adds gsub('%{env}', deploy_env) to the SP seeder and that text is not present in the YAML file) but will try to button up testing tomorrow!

@orenyk orenyk force-pushed the oyk-env-specific-yaml branch from b5bafe8 to 3ac0760 Compare February 7, 2023 02:49
@orenyk
Copy link
Contributor Author

orenyk commented Feb 7, 2023

Also, updated this to account for @julialeague's comment in https://github.com/18F/identity-idp-config/pull/1256 - this modifies the seeder behavior for cases where restrict_to_deploy_env is equal to sandbox and I believe otherwise remains the same.

Note that the updated service_providers.yml file will ultimately require restrict_to_deploy_env for all configurations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please double check me here but I'm 99% that lines 63-68 are the same, functionally, as the previous code, with the addition of line 65 (restrict_to_deploy_env set to sandbox). I found the old code hard to read / reason about so even though this is more verbose I think it's a bit clearer.

Copy link
Contributor

Choose a reason for hiding this comment

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

very minor, but my immediate thought when glancing at this was that it could be rewritten as a case statement. Either way works, though!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I generally like guard clauses over case statements for Ruby but I had the same thought myself 😄

@orenyk orenyk changed the title [LG-6146] Add support for env-specific service_providers.yml files [LG-6146] Prepare for removing ERB from service_providers.yml Feb 8, 2023
Resolves LG-6146

changelog: Internal, Deployment, Add support for environment-specific content in configuration
@orenyk orenyk force-pushed the oyk-env-specific-yaml branch from 3ac0760 to 8ffb9fb Compare February 8, 2023 17:09
@orenyk
Copy link
Contributor Author

orenyk commented Feb 8, 2023

Tested this in a personal sandbox (thanks @mitchellhenke) and it appeared to work fine, so going to merge this in for tomorrow's release

@orenyk orenyk merged commit 7125934 into main Feb 8, 2023
@orenyk orenyk deleted the oyk-env-specific-yaml branch February 8, 2023 22:43
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