-
Notifications
You must be signed in to change notification settings - Fork 2
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
PRODENG-1577: Kube deployment #1687
Conversation
101e65b
to
8e3a75e
Compare
@raynorg I see you managed to get a Green build...How are we looking in terms of getting this merged? |
a0aa07c
to
a3a468d
Compare
c8985ea
to
cbf45f5
Compare
e990529
to
94ef753
Compare
72d1444
to
1d03b45
Compare
Just a heads up I have merged #1641, which updates Ruby to v2.7.7 and Rails to 6.0.6 |
1d03b45
to
a53e325
Compare
a53e325
to
02a502c
Compare
b5f45fa
to
d0f2ff2
Compare
11dde9c
to
48f6933
Compare
author George MacLean <[email protected]> 1675203466 -0700 committer Shane Smith <[email protected]> 1706293936 -0600 parent 633d0cb author George MacLean <[email protected]> 1675203466 -0700 committer Shane Smith <[email protected]> 1706293885 -0600 PRODENG-1577: Push containers to ECR Amending the correct build/deploy file might not need this guy updating root URLs for testing kube deployment updating dockerfile, and fixing typo in application.rb might be easier nope adding tag release image job Updating root URLs for new DNS zone nope Rebased, added development specific release-deploy job Adding Production URLS to application.rb Renaming development job, and removing prod/development tags from primary release GH action One more push to all environments please
a57ec4c
to
da97607
Compare
Dependency Review✅ No vulnerabilities or license issues found.Scanned Manifest Files.github/workflows/release-deploy.yml
.github/workflows/deploy.yml
.github/workflows/release-deploy-v2.yml
.github/workflows/release-deploy-v3.yml
.github/workflows/tag-release-image.yml
docs/package.json
yarn.lock
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. A few quick questions, but I'm good with no action responses, just curious:
- Why set the "default" value to production values vs. using the environmental config?
- If we're dropping the qa/dev environments, why not remove those environmental config files?
Nice work getting this over the line @voodooGQ @raynorg @ju-Skinner 👍
|
||
# Sassdocs deploy url to differentiate in different environments | ||
config.sassdocs_root_url = "https://sage-lib-sassdocs.herokuapp.com/" | ||
config.sassdocs_root_url = "https://sage-lib-sassdocs.production.kajabi.farm/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these being set here vs. in the config/environments/production.rb
file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@prsimp
I am not disagreeing with you, but the current implementation reads from the config files.
https://github.com/Kajabi/sage-lib/blob/maindocs/app/views/application/_home_code.html.erb#L14
https://github.com/Kajabi/sage-lib/blob/main/docs/app/views/application/_footer.html.erb#L22
https://github.com/Kajabi/sage-lib/blob/main/docs/app/helpers/application_helper.rb#L38
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Soooo..... leave as is? Or change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@prsimp dev !== development. There was some finagling to get it to work right on Argo, but now that we're not using dev
as an env the dev
references have been removed. development
is still in use for local.
@ju-Skinner mentioned that QA was in use in some fashion, but it's not a deployed environment. Happy to remove the rb
file too if that's what we want to do, but would need an approval from @ju-Skinner
- main | ||
- develop | ||
- PRODENG-1577/kube_deployment | ||
jobs: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason lint,test, and build
were removed from this workflow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a net-new file, so not sure how it was "removed". Do you want me to add the job as a dep?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I wasn't clear. In comparison to release-deploy-v2
, we no longer run those in this workflow. Is there a reason it wasn't included?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mainly because it wasn't in there when I took over. I didn't really go through all the details because we had some environments working, my goal was simply to get the rest of them working. So, no, no particular reason it wasn't included. Happy to add it if that's what you want, just give me the word.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going through the file diff here I don't see lint/test/build
being referenced in any of the old ones. I do see this but that's referring to the lint
, test
, and build
steps in that specific workflow.
Not saying lint/test/build would be a bad thing by any means, but just wondering if it's actually necessary? Your develop
and main
are protected right (Could be wrong about that, legitimately asking)? And lint/test/build
fires on PR, so it would have ran before it got merged technically. Doesn't cover all edge cases but it should work as expected with that kind of setup right?
Either way I'm fine adding it, couple line change and adds some extra protection. But not sure if it's strictly necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.github/workflows/release-deploy.yml
Outdated
- develop | ||
- PRODENG-1577/kube_deployment | ||
jobs: | ||
release_deploy: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having release_deploy
as one big job will require rerunning the whole process should one step fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you like here? I have no stake in this game, so need some direction on how you'd like it handled. Otherwise feel free to make changes and commit to this PR if you have a game plan.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it resemble what was in release-deploy-v2
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
docs/Dockerfile
Outdated
@@ -8,6 +8,10 @@ ARG NODE_VERSION="16.20.2" | |||
ARG YARN_VERSION="1.22.18" | |||
ARG ARCH="x64" | |||
ARG GITHUB_TOKEN | |||
ARG SECRET_KEY_BASE="foobarbaz" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the value we want to use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷 it's what was in there. Would you like something different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷🏾 just called it out. I don't know either. lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You normally need some value in that environment variable when running any asset precompile or rails command. It's set via ENV var in most apps in a production environment, but you just have to have something. It could be changed to "setting_random_secret_for_asset_precompile"
if you want
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ This is/was used for the current Heroku deployments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
K. Won't be necessary once we merge and verify, so think it's safe to leave out. But you let me know.
Forgot we merged the Node 18 PR into develop, so gotta go through and update the Dockerfiles. BRB... |
Nevermind, can't really do that until the Node 18 branch is in |
2257f24
to
ed1555c
Compare
This is mergable at this point, waiting for some information on the Kubernetes side before we merge that one, and then we'll merge this one. Doing it in this order so we don't drop the Heroku stuff before the K8s is ready. |
K8s PR merged: https://github.com/Kajabi/kubernetes/pull/1103 This should be ready to roll. Going to merge in, verify it ran on develop as it should, then reach out to PE to swap the DNS |
From @voodooGQ:
Alright, I think we're good to go. After many discussions with @ju-Skinner and @raynorg we decided to strip the
qa
anddev
environment from the deploys. Dev doesn't make sense because dev is not a production-like environment. QA is not really needed because they'll verify via Kajabi Products through Proxy.Tested on
staging
seems to be working:https://sage-lib-sassdocs.staging.kajabi.farm/
https://sage-lib-documentation.staging.kajabi.farm/pages/index
https://sage-lib-storybook.staging.kajabi.farm/?path=/story/sage-accordion--single-panel
This is in conjunction with https://github.com/Kajabi/kubernetes/pull/1103, I'm assuming we need to get this one in play first or K8s will have nothing to pull, but let me know if we need to do it in reverse.
Will be getting that PR ready to be reviewed next.(EDIT: Ready to roll)Things did:
v2
andv3
of the release which from all I gathered are not needed. Addedmain
anddevelop
to the main release workflow with a switcher to tag the right image path. Not really sure if thetag-release
one is needed, but I guess it can't hurt since it's just retagging via a dispatch 🤷docs/Dockerfile
so it would play nice on K8sdeploy
workflow that was launching to Heroku 🎉@babel/plugin-proposal-private-property-in-object
in the docs packages because it was barking 🐶@raynorg since you made the PR I can't add you to review, added you to
Assignees
instead so you can peek over this.