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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix push script #4249

Merged
merged 5 commits into from
Apr 22, 2024
Merged

fix push script #4249

merged 5 commits into from
Apr 22, 2024

Conversation

jLopezbarb
Copy link
Contributor

@jLopezbarb jLopezbarb commented Apr 18, 2024

Proposed changes

DEV-288

There was an issue in the regex check. We were comparing if the string started with beta but we had the circle tag starting with 2.26.0.

Remove the ^ on the regex in order to check the whole string and not just if it starts with beta

How to validate

  1. Comment the line 40 (okteto build --platform "${PLATFORMS}" --build-arg VERSION_STRING="${RELEASE_TAG}" -t "${tags}" -f Dockerfile .)
  2. Run ./scripts/ci/push-image.sh 2.26.0 "linux/amd64,linux/arm64" and check that the message is Pushing okteto/okteto:2.26.0,okteto/okteto:dev,okteto/okteto:stable to Docker Hub
  3. Run ./scripts/ci/push-image.sh 2.26.0-beta.7 "linux/amd64,linux/arm64" and check that the message is Pushing okteto/okteto:2.26.0-beta.7,okteto/okteto:dev,okteto/okteto:beta to Docker Hub

CLI Quality Reminders 馃敡

For both authors and reviewers:

  • Scrutinize for potential regressions
  • Ensure key automated tests are in place
  • Build the CLI and test using the validation steps
  • Assess Developer Experience impact (log messages, performances, etc)
  • If too broad, consider breaking into smaller PRs
  • Adhere to our code style and code review guidelines

Signed-off-by: Javier Lopez <javier@okteto.com>
Signed-off-by: Javier Lopez <javier@okteto.com>
@jLopezbarb jLopezbarb requested a review from a team as a code owner April 18, 2024 11:56
@jLopezbarb jLopezbarb changed the title Jlo/fix push script fix push script Apr 18, 2024
@jLopezbarb jLopezbarb added release/bug-fix backport release-2.26 Backport this PR to CLI version 2.26 labels Apr 18, 2024
@@ -23,16 +23,16 @@
fi


beta_prerel_regex="^beta\.[0-9]+"
beta_prerel_regex="beta\.[0-9]+"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it really the change in the regex that fixes this and not changing in the order if/elif? This regex is checked against the prerel so it will never have the MAYOR.MINOR.PATCH

Copy link
Member

Choose a reason for hiding this comment

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

Agree, I think it is not needed that change. Not sure if it might break something or not. Given the current format of tag versioning it shouldn't be needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I needed to do it with my semver binary but now using the binary that we have in CI it seems to work as expected. Changing it

Signed-off-by: Javier Lopez <javier@okteto.com>
Copy link

codecov bot commented Apr 18, 2024

Codecov Report

Merging #4249 (6b9a844) into master (e4fc052) will decrease coverage by 0.03%.
Report is 3 commits behind head on master.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4249      +/-   ##
==========================================
- Coverage   45.60%   45.58%   -0.03%     
==========================================
  Files         305      305              
  Lines       27602    27602              
==========================================
- Hits        12589    12583       -6     
- Misses      13940    13944       +4     
- Partials     1073     1075       +2     

Copy link
Member

@ifbyol ifbyol left a comment

Choose a reason for hiding this comment

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

Changes looks good, I've been testing it and it works as expected. The only thing is the change in the regex. Don't think is needed as discussed inline, but I don't think it is a blocker to merge this

@@ -23,16 +23,16 @@
fi


beta_prerel_regex="^beta\.[0-9]+"
beta_prerel_regex="beta\.[0-9]+"
Copy link
Member

Choose a reason for hiding this comment

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

Agree, I think it is not needed that change. Not sure if it might break something or not. Given the current format of tag versioning it shouldn't be needed

prerel="$(semver get prerel "${RELEASE_TAG}" || true)"
version="$(semver get release "${RELEASE_TAG}" || true)"
tags="okteto/okteto:${RELEASE_TAG},okteto/okteto:dev"

Copy link
Member

Choose a reason for hiding this comment

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

Nit: Would it be helpful to print prerel and version for debug purposes? It was really hard to debug the problem in the CI job as we don't log anything. I think it might be useful to print RELEASE_TAG value plus the other 2 vars

Signed-off-by: Javier Lopez <javier@okteto.com>
Signed-off-by: Javier Lopez <javier@okteto.com>
@jLopezbarb jLopezbarb merged commit a46a9da into master Apr 22, 2024
12 checks passed
@jLopezbarb jLopezbarb deleted the jlo/fix-push-script branch April 22, 2024 10:04
github-actions bot pushed a commit that referenced this pull request Apr 22, 2024
* fix: push script

Signed-off-by: Javier Lopez <javier@okteto.com>

* refactor: uncomment push

Signed-off-by: Javier Lopez <javier@okteto.com>

* fix: use the same semver as ci

Signed-off-by: Javier Lopez <javier@okteto.com>

* fix: add beta regex

Signed-off-by: Javier Lopez <javier@okteto.com>

* feat: add debug logs

Signed-off-by: Javier Lopez <javier@okteto.com>

---------

Signed-off-by: Javier Lopez <javier@okteto.com>
(cherry picked from commit a46a9da)
jLopezbarb added a commit that referenced this pull request Apr 22, 2024
* fix: push script

Signed-off-by: Javier Lopez <javier@okteto.com>

* refactor: uncomment push

Signed-off-by: Javier Lopez <javier@okteto.com>

* fix: use the same semver as ci

Signed-off-by: Javier Lopez <javier@okteto.com>

* fix: add beta regex

Signed-off-by: Javier Lopez <javier@okteto.com>

* feat: add debug logs

Signed-off-by: Javier Lopez <javier@okteto.com>

---------

Signed-off-by: Javier Lopez <javier@okteto.com>
(cherry picked from commit a46a9da)

Co-authored-by: Javier L贸pez Barba <javier@okteto.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport release-2.26 Backport this PR to CLI version 2.26 release/bug-fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants