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.

Sign up for GitHub

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

Jump to bottom

[OCU-90] Make Dracon-dev into one Helm Package #151

Merged
merged 1 commit into from
May 4, 2024
Merged

[OCU-90] Make Dracon-dev into one Helm Package #151

merged 1 commit into from
May 4, 2024

Conversation

northdpole
Copy link
Contributor

@northdpole northdpole commented Apr 25, 2024 •
edited
Loading

Implements #150

ptzianos
ptzianos requested changes Apr 28, 2024
View reviewed changes
Copy link
Contributor

@ptzianos ptzianos left a comment

Choose a reason for hiding this comment

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

Please split away the RFC part into a separate PR, we first review and merge that, then we can proceed to reviewing this, but only after it's working and the commit history has been cleaned up

@northdpole northdpole force-pushed the helmify-migrations branch 3 times, most recently from 7d1911d to 7aa8c7f Compare April 29, 2024 13:43
@northdpole northdpole changed the title [OCU-90] helmify rfc [OCU-90] Make Dracon-dev into one Helm Package Apr 29, 2024
@northdpole
Copy link
Contributor Author

northdpole commented Apr 29, 2024 •
edited
Loading

Please split away the RFC part into a separate PR, we first review and merge that, then we can proceed to reviewing this, but only after it's working and the commit history has been cleaned up

thanks for the review, done, RFC PR

@northdpole northdpole mentioned this pull request Apr 29, 2024
Closed
@ptzianos ptzianos force-pushed the main branch 2 times, most recently from c21da37 to c288d4b Compare April 30, 2024 12:41
ptzianos
ptzianos requested changes May 2, 2024
View reviewed changes
Copy link
Contributor

@ptzianos ptzianos left a comment

Choose a reason for hiding this comment

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

5 nits and we are good to go 🚀

Makefile Outdated
@@ -239,4 +208,24 @@ deploy-tektoncd-dashboard: tektoncd-dashboard-helm
--values ./deploy/tektoncd/dashboard/values.yaml \
--namespace $(TEKTON_NS)

dev-deploy: deploy-nginx deploy-arangodb deploy-kibana deploy-mongodb deploy-pg deploy-tektoncd-pipeline deploy-tektoncd-dashboard
deploy-dracon-dev: deploy-elasticoperator
make draconctl-image-publish CONTAINER_REPO=localhost:5000/ocurity/dracon
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this, the user can run this command themselves if needed

Makefile Outdated
make deploy-dracon-dev

dev-teardown:
kind delete clusters dracon-demo
Copy link
Contributor

Choose a reason for hiding this comment

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

forgot an @ over here

Makefile Outdated
--set "global.image.draconVersion=$(DRACON_VERSION)"
--wait

dev-deploy:
Copy link
Contributor

Choose a reason for hiding this comment

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

you can simplify this to dev-deploy: deploy-nginx deploy-arangodb deploy-tektoncd-pipeline deploy-tektoncd-dashboard deploy-dracon-dev

deploy/dracon/charts/elasticsearch-0.1.0.tgz Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should add this folder to the .gitignore these will be pulled when the help install/upgrade command is executed

deploy/dracon/templates/job.yaml Outdated
@@ -0,0 +1,29 @@
apiVersion: batch/v1
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we should add a boolean to enable migrations and have the entire job in an if so that if for whatever reason the migrations are not supposed to be deployed, they can be skipped

@ptzianos ptzianos force-pushed the helmify-migrations branch 2 times, most recently from 458c251 to ce87b83 Compare May 4, 2024 18:52
@northdpole @ptzianos
fixes 114,150: create a single Helm package for dracon-dev and releva…
82771f3 
…nt make targets

This commit unifies the installation of dracon-dev and its dependencies
via one single helm package. It installs all necessary dependencies for
development such as Elasticsearch, Kibana, Postgres, Mongo and Tekton.
Dependencies that need to be installed on a namespace different than the
dracon components such as Tekton, Nginx, ElasticOperator and ArangoDB are
installed via Make targets.

This commit also refactors Make targets introducing `deploy-dracon-dev`
which builds and loads dracon container images and installs the helm
chart.

Last this commit runs the necessary dracon migrations job as a helm
post-install hook
@ptzianos ptzianos merged commit 82771f3 into main May 4, 2024
3 checks passed
@ptzianos ptzianos deleted the helmify-migrations branch May 4, 2024 19:03
@ptzianos ptzianos mentioned this pull request May 4, 2024
Closed
@ptzianos ptzianos mentioned this pull request May 4, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ptzianos ptzianos ptzianos requested changes

@flowirtz flowirtz Awaiting requested review from flowirtz

@dlicheva dlicheva Awaiting requested review from dlicheva

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@northdpole @ptzianos