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

update the publish workflow to also build and publish dracon itself a… #185

Closed
wants to merge 2 commits into from

Conversation

northdpole
Copy link
Contributor

@northdpole northdpole commented May 15, 2024

…nd draconctl

closes #186

@northdpole northdpole requested a review from ptzianos May 15, 2024 13:49
@@ -4,7 +4,6 @@ description: |
A Helm chart for Kubernetes containing services needed for Dracon pipelines to run. Please check the documentation for more information
type: application
version: 0.7.0
Copy link
Contributor

Choose a reason for hiding this comment

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

since you removed the appVersion you can remove the version field too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

every helm chart needs a version, can't remove it

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 pass the version as a parameter when building the chart same way you do for the appVersion

Makefile Outdated Show resolved Hide resolved
.github/workflows/publish.yml Show resolved Hide resolved
@northdpole northdpole force-pushed the improve-publish-action branch 2 times, most recently from 445d024 to 4df7507 Compare May 15, 2024 16:40
Makefile Show resolved Hide resolved
set -e
DRACON_VERSION_SEMVER=$(sed 's/v//' <<< ${{ github.ref_name }})

go install github.com/ocurity/dracon/cmd/draconctl@latest
Copy link
Contributor

Choose a reason for hiding this comment

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

why do this? you already checked out the latest version of the codebase and you are executing commands in it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's a matter of principle:
in this scope draconctl is a tool used to package and publish things, hence we use the latest stable version

if we were to be testing draconctl then we can build it from the binary
i don't see how this is a problem

Copy link
Contributor

Choose a reason for hiding this comment

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

you are not using the latest stable version, you are using the latest commit in the main branch. these are not the same unfortunately

go install github.com/ocurity/dracon/cmd/draconctl@latest
~/go/bin/draconctl components package --version ${{ github.ref_name }} --chart-version ${DRACON_VERSION_SEMVER} --name dracon-oss-components ./components;

helm package --version=${DRACON_VERSION_SEMVER} --app-version=${DRACON_VERSION_SEMVER} deploy/dracon/chart
Copy link
Contributor

Choose a reason for hiding this comment

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

the --app-version should be set to ${{ github.ref_name }} in the same way as it is in the previous command so that we can reference the original tag of the git repo

Makefile Outdated
@@ -211,6 +213,7 @@ dev-dracon: deploy-elasticoperator deploy-arangodb-crds add-bitnami-repo
--values ./deploy/dracon/values/dev.yaml \
--create-namespace \
--namespace $(DRACON_NS) \
--version $$(echo "${DRACON_VERSION}" | sed 's/^v//')
Copy link
Contributor

Choose a reason for hiding this comment

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

the line has a different format than the rest of the command and it's missing a slash at the end. there is a small bug at the next line that's also missing a slash at the end, unrelated to this PR but I think we should fix it since we are working here.

@@ -64,7 +64,14 @@ enrichmentDB:
# enrichment database
connectionStr: ""

dracon-components:
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 see any change in the Helm manifests where this is used. why is it here?

global:
image:
# registry to use for all
registry: ""
registry: "ghcr.io"
Copy link
Contributor

Choose a reason for hiding this comment

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

all our images already have a default value for the registry in the Helm template, I don't think we need this here or if we put it here, we should remove the use of the default function in the Helm templates

@@ -13,4 +11,4 @@ dependencies:
- name: mongodb
version: 15.1.5
repository: https://charts.bitnami.com/bitnami
condition: mongodb.enabled
condition: mongodb.enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

why remove the last line?

@northdpole northdpole closed this Jun 4, 2024
@ptzianos ptzianos deleted the improve-publish-action branch September 21, 2024 23:33
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.

Make Publish job also publish draconctl and dracon itself
2 participants