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

changing image tag GITHUB_ENV #747

Closed
wants to merge 1 commit into from
Closed

changing image tag GITHUB_ENV #747

wants to merge 1 commit into from

Conversation

armin-x86
Copy link

@armin-x86 armin-x86 commented Dec 25, 2023

issue #746

GITHUB_REF_NAME The short ref name of the branch or tag that triggered the workflow run. This value matches the branch or tag name shown on GitHub. For example, feature-branch-1.

For pull requests, the format is refs/pull/<pr_number>/merge.

@@ -233,7 +233,7 @@ jobs:

- name: Prepare Docker environment
run: |
echo IMAGE_TAG=$(if [ "$GITHUB_REF" == "refs/heads/main" ]; then echo "main-${GITHUB_SHA::7}"; else echo "$GITHUB_REF_NAME"; fi) >> $GITHUB_ENV
Copy link
Member

Choose a reason for hiding this comment

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

The old thingy looks correct to me what's the issue with having commits that isn't released tagged as main-<commit hash>?

I let @sergejparity answer this because I don't understand the issue most likely there is something in the deploy logic that requires a version somewhere? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@arminteimouri change you propose just flips then-else condition. But the result will be the same.

Actually mine solution came from preposition that IMAGE_TAG's like main-xxxxxxx will happen more often and I've put it to the first place.

Copy link
Author

Choose a reason for hiding this comment

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

The old thingy looks correct to me what's the issue with having commits that isn't released tagged as main-<commit hash>?

check it – peep the image name, right? That's the key to knowing what version we're running on GKE. But hold up, when you glance at the code, it's like, "What version is this?" 'Cause it's all about the commit hash, and that ain't telling us squat. Sure, we can dive into the repo, track down the commit, find the tag – but why all that hassle? We can keep it chill and just make the version clear from the get-go

Copy link
Contributor

@sergejparity sergejparity Dec 25, 2023

Choose a reason for hiding this comment

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

@arminteimouri probably @niklasad1 trying to tell, that this PR actually doesn't change anything apart from sequence of how if condition is evaluated.

it was:

if (push to branch `main`)
  IMAGE_TAG="main-xxxxsha"
else
 IMAGE_TAG="v0.x.x"
fi

it became:

if (tag pushed)
  IMAGE_TAG="v0.x.x"
else
 IMAGE_TAG="main-xxxxsha"
fi

That docker image naming logic wasn't introduced by me. I just refactored former GitLab CI into GitHub Action.

Copy link
Member

@niklasad1 niklasad1 Dec 25, 2023

Choose a reason for hiding this comment

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

Alright, I see.

We could rename "main-xxxxsha" to "v1.3.0-unstable-xxxxsha" instead, I reckon that would help to understand which version you want to deploy right?

v1.3.0 is the unreleased version in this example and could be parsed from https://github.com/paritytech/polkadot-staking-miner/blob/main/Cargo.toml#L3

Copy link
Author

@armin-x86 armin-x86 Dec 25, 2023

Choose a reason for hiding this comment

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

won't that tweak make the pushed Docker image name match the tag and be easy to know which release tag it's linked to?
we got the GITHUB_REF_NAME in the mix, rolling as the image name.

that this PR actually doesn't change anything apart from sequence of how if condition is evaluated

Switching up that sequence does the trick, letting the image tag snag the victory over the main-commitHash name and getting elected as the image name. Correct me please if I'm mistaken.

Copy link
Author

Choose a reason for hiding this comment

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

We could rename "main-xxxxsha" to "v1.3.0-unstable-xxxxsha" instead,

That make things much clearer and easier to read.

Copy link
Contributor

Choose a reason for hiding this comment

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

Switching up that sequence does the trick, letting the image tag snag the victory over the main-commitHash name and getting elected as the image name. Correct me please if I'm mistaken.

Not really, this job can distinguish triggering event e.g. push to the main branch in this case "$GITHUB_REF" == "refs/heads/main"
And second case is when tag is pushed.
"$GITHUB_REF" == "refs/tags/vX.X.X"

That's why I'm talking, that you just swapped checks, and they are not overlapping, so order doesn't matter.

@niklasad1
Copy link
Member

Closing this as stale and it could be useful but not really a priority for now

@niklasad1 niklasad1 closed this Apr 5, 2024
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