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

Add a build step to deploy to EKS #11231

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

Add a build step to deploy to EKS #11231

wants to merge 8 commits into from

Conversation

voidlily
Copy link
Contributor

The eks-control repo orchestrates deployments to different envs, going through dev->staging->prod, with manual steps to go staging and prod

@voidlily voidlily force-pushed the deploy-to-eks branch 3 times, most recently from b756df2 to 93e8755 Compare September 16, 2024 19:09
project: lg-public/identity-eks-control
# TODO change branch to main before merge
branch: lrappaport/deploy-eks
stage: deploy_eks
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a specific job this stage depends on? Can we use needs to be more precise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without the use of needs, the dependency is "all jobs in previous stages"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you look at https://gitlab.login.gov/lg/identity-idp/-/pipelines/120712 you can see it's currently the rightmost stage

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, sorry, I understand that aspect. This job is dependent on deploy_production?

I didn't quite understand the interaction with the deploy to "production" there with the description of deploying to different environments in the PR description.

Copy link
Member

@timothy-spencer timothy-spencer Sep 17, 2024

Choose a reason for hiding this comment

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

Yeah, we should seriously consider renaming the deploy_production job to deploy_reviewapp or something. It's not production, and definitely can be confusing. Probably also need to remove it's environment.name: production stuff too.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's kind of why I lean towards using needs to explicitly define the dependencies. I think it's worth renaming deploy_production as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i guess, my main requirements here are "everything" and if later on we add another step to the test phase i want that to automatically get picked up as a requirement? hence the stages thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually, doesn't the main reviewapp go away now? it would be the same as a continuously deployed dev env that this pipeline also adds?

https://idp.eksdev.identitysandbox.gov/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if there's no objection i'm just gonna delete that step entirely

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd defer to @timothy-spencer on that aspect.

The eks-control repo orchestrates deployments to different envs, going through
dev->staging->prod, with manual steps to go staging and prod

Changelog: Internal, Deploy, deploy to EKS
This step was actually deploying the `main` branch reviewapp, but we don't need
that anymore with this PR because there's a long lived main branch env now at https://idp.eksdev.identitysandbox.gov
this works for eks because it's `pgbouncer.namespace.svc.cluster.local`
@voidlily voidlily force-pushed the deploy-to-eks branch 2 times, most recently from c6056ae to fbc1bcd Compare September 18, 2024 22:35
Comment on lines 17 to 18
database_primary_advisory_locks: false
database_primary_prepared_statements: false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this needs to be in the actual configs and i don't know how to do that, how do you do that?

@voidlily
Copy link
Contributor Author

was getting 500 errors around the prepared statements and was trying to figure out why, so i went so far as to make a truth table to figure this out

            value true | value false
socket yes      ?            F
socket no       T            F

if the new config value is supposed to take precedence, then it doesn't matter what the value of the db socket is and we can remove that from the database.yml (and make sure the config change rolls out first before the code change)

if the new config value doesn't take precedence, it means the || should be a &&

@voidlily
Copy link
Contributor Author

if we want to be more careful with rolling this out we could do a 2 phased thing, first one the old config value takes precedence, then a second wave to remove it once the config change are confirmed out

next PR remove the socket present thing entirely
@voidlily voidlily marked this pull request as ready for review September 19, 2024 17:03
@mitchellhenke
Copy link
Contributor

if we want to be more careful with rolling this out we could do a 2 phased thing, first one the old config value takes precedence, then a second wave to remove it once the config change are confirmed out

I think it's fine to use the config directly right away as long as the default for both is false.

@voidlily
Copy link
Contributor Author

voidlily commented Sep 19, 2024

default if it's not in the config will raise an error about it not existing so this is already a "breaking change" https://github.com/18F/identity-hostdata/blob/main/lib/identity/hostdata/config_builder.rb#L33

so since we already have to eat a tricky change here yeah we can roll it all together in one commit

voidlily and others added 2 commits September 19, 2024 10:32
identity-hostdata raises an error if the key is missing anyway so it doesn't
matter that this is technically a breaking change
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.

3 participants