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!: remove use_release_candidate option #1437

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jthegedus
Copy link
Contributor

@jthegedus jthegedus commented Jan 18, 2023

Summary

Now we are using release-please & GitHub Releases to manage our changelog generation and release pipeline we're no longer using release candidates (as they were originally intended) with asdf core.

People who wish to test the latest code should use HEAD via asdf update --head.

If HEAD is too unstable for people and they want an RC equivalent, then we have two options:

  1. we can implement asdf update <sha>
  2. we can recommend users cd to asdf installation and perform a manual git checkout <sha>

Given no users have raised missing the RC I feel we are safe to go with option 2 here. We can add asdf update <sha> in future if it becomes necessary to do so.

Other Information

This also removes sort_versions from the repository 馃帀

Related:

Fixes #1515

@jthegedus jthegedus requested a review from a team as a code owner January 18, 2023 14:54
@jthegedus
Copy link
Contributor Author

jthegedus commented Jan 18, 2023

Big question before we merge, is this considered a "breaking change"? I would think yes, as we're changing an external API, though it is only for users to update, but it could be used in a CI pipeline? 馃

Thoughts @Stratus3D

Edit: I have marked as Breaking with ! in the conventional-commit type. I still wish to get your thoughts.

@jthegedus jthegedus changed the title fix: remove asdf use_release_candidate option fix!: remove asdf use_release_candidate option Jan 18, 2023
# Exclude RC tags when selecting latest tag
tag=$(git tag | sort_versions | grep -vi "rc" | sed '$!d') || exit 1
fi
sha_of_tag=$(git rev-list --tags --max-count=1) || exit 1
Copy link
Member

Choose a reason for hiding this comment

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

Is this git rev-list command returning the most recently created tag, or the tag with the highest semantic version?

If this is returning the most recently created tag, we may want to stick with the sort_versions approach here in case we ever tag a new patch version for old minor version of asdf. I know we've not done that thus far, so maybe it's not necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, looking at this again I'd suggest keeping sort_versions for now. All other changes look good to me.

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 have a different solution to propose here which would still remove sort_versions, I have just been busy with work so haven't had a chance to write it up. I would like to discuss that alternative before keeping sort_versions

Copy link
Member

@Stratus3D Stratus3D left a comment

Choose a reason for hiding this comment

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

I feel like the old approach with sort_versions was more foolproof, as it always returned the right tag. It seems like the new code could return any tag, regardless of what name the tag was, as long as it was the newest. If someone were to create an rc or beta tag for some reason it'd be recognized as the latest. I know we don't intend to do that anymore, but it might be better to play it safe.

I think getting rid of use_release_candidate is the right choice, other than my concern with sort_versions this looks great!

@jthegedus
Copy link
Contributor Author

When I created this change I was not considering the scenario where we would manually create rc or beta tags.

I will update this PR with another suggestion.

@Stratus3D
Copy link
Member

Big question before we merge, is this considered a "breaking change"?

I guess I never answered your question yesterday. Yes, I'd consider this a breaking change just to be safe.

@jthegedus jthegedus self-assigned this Feb 19, 2023
@jthegedus jthegedus added the priority asdf core intend to resolve soon label Feb 19, 2023
@jthegedus jthegedus changed the title fix!: remove asdf use_release_candidate option fix!: deprecate use_release_candidate option Apr 6, 2023
@jthegedus jthegedus changed the title fix!: deprecate use_release_candidate option fix!: remove use_release_candidate option Apr 6, 2023
@jthegedus
Copy link
Contributor Author

jthegedus commented Apr 6, 2023

My last push was to just rebase before my new changes. I won't get to proposing my new changes until next week (2nd week April 2023).

The gist of my upcoming changes are:

  • prepare
    • detect default branch
    • detect remote
    • git fetch from remote
  • asdf update: use GitHub Release to detect "latest" version, then checkout the tag (as we do now)
    • manual GH Releases do NOT default to "latest", that is a checkbox to manually opt-in
    • automated release-please GH Releases will assign "latest"
  • asdf update --head: checkout default branch and reset to HEAD (as it does now)
  • asdf update <sha|tag>: checkout the specific SHA or Tag

Still working through updating tests cases etc

While certainly more code than what we currently have, I feel this is more straightforward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority asdf core intend to resolve soon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: asdf update --head odd behavior
2 participants