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

Fixes lagging version of jimm release in our CI/CD #1054

Closed
wants to merge 2 commits into from
Closed

Fixes lagging version of jimm release in our CI/CD #1054

wants to merge 2 commits into from

Conversation

mina1460
Copy link
Contributor

@mina1460 mina1460 commented Oct 12, 2023

Description

Multiple issues reported issues with fetching tags and latest versions in older actions/checkout version.
see actions/checkout#579 and actions/checkout#448

Hopefully by migrating to the new checkout v4 and forcing fetch-depth to 0 and asking for fetch-tags can solve the issue. In some cases, I add a separate command to git fetch --tags explicitly which should come at no cost given that tags should already be fetched in the previous steps.

Fixes CSS-5482

Engineering checklist

Check only items that apply

  • Documentation updated
  • Covered by unit tests
  • Covered by integration tests

Test instructions

We need to push a new tag and check. this can be unit-tested or anything similar

Signed-off-by: mina1460 <mina146@aucegypt.edu>
with:
fetch-depth: 0
fetch-tags: true
- run: git fetch --tags --prune --unshallow
Copy link
Member

Choose a reason for hiding this comment

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

This may cause confusion because we've already fetched the tags (by fetch-* fields), so if it's not a necessary thing I think it's best to drop it.

Of course, except if you've already checked that and it should be there for things to work as expected. If this is the case, a simple comment would be great.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that it shouldn't be necessary, but people seem to be recommending this as a workaround in multiple issues online. Including this popular one
actions/checkout#701 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Iirc I think they fixed this, also that's v3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I think they fixed it in v4 so I moved all actions to v4 @ale8k

with:
fetch-depth: 0
fetch-tags: true
- name: Get latest tags
run: git fetch --tags
Copy link
Member

Choose a reason for hiding this comment

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

Here, 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.

it is just an extra step that costs us practically nothing just to be extra sure

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with babak though, if it isn't needed, just remove it, I feel like fetch-tags will work

- uses: actions/checkout@v3
- run: git fetch --prune --unshallow
- uses: actions/checkout@v4
- run: git fetch --tags --prune --unshallow
Copy link
Member

Choose a reason for hiding this comment

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

And here.

@@ -18,8 +18,8 @@ jobs:
build-charm:
runs-on: ubuntu-20.04
steps:
- uses: actions/checkout@v3
- run: git fetch --prune --unshallow
- uses: actions/checkout@v4
Copy link
Member

Choose a reason for hiding this comment

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

Why fetch-* options are not used here (i.e., in charm.yaml), but have been used in charm test actions? I imagine both should have the same interest in git tags, right?

with:
fetch-depth: 0
fetch-tags: true
- name: fetch newest tags
run: git fetch --tags
Copy link
Member

Choose a reason for hiding this comment

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

Here, too.

with:
fetch-depth: 0
fetch-tags: true
- run: git fetch --tags --prune --unshallow
Copy link
Member

Choose a reason for hiding this comment

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

Here, too.

@kian99
Copy link
Contributor

kian99 commented Oct 12, 2023

This PR should go into main and then we can bring it into feature-rebac when we merge in main. I'm working on a PR now to bring other previous changes from main to `feature-rebac

@mina1460 mina1460 closed this Oct 12, 2023
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.

5 participants