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

Add support for non-tagged publishing #35

Merged
merged 4 commits into from
Feb 18, 2024
Merged

Add support for non-tagged publishing #35

merged 4 commits into from
Feb 18, 2024

Conversation

kip93
Copy link
Contributor

@kip93 kip93 commented Nov 5, 2023

Modifies the publish API endpoint so that it does not assume that the release being created is using a tag. For this, it adds a ref input that points to the commit to be released. Should be backwards compatible since if ref is not provided it defaults to f"refs/tags/{version}".

To make this work, had to change the order in which some operations were done. And modified a 400 code to 409 since it was more appropriate and it allows the caller to know why it failed and potentially ignore the error (see flakestry/flakestry-publish#2).

Finally, since I could not for the life of me create an OIDC JWT ID for testing, I only tested the API call to GitHub separately and could not test the endpoint as a whole.

Closes #26

elif publish.ref and publish.ref.startswith("refs/tags/"):
given_version = publish.ref.removeprefix("refs/tags/")
else:
given_version = f"v0.1.{re.sub(r'[^0-9]', '', commit_date)}"
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we should have a version in this case. Maybe the user should provide a field like version without the minor - that would automatically be a unix timestamp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we can change it to accept strings that can be then formatted like so

datetime = re.sub(r'[^0-9]', '', commit_date)
version = publish.version.format(datetime=datetime)

and they can provide a value such as v1.0.{datetime}, which results in v1.0.20231106161132.

This could be extended to get some nicer formats too

datetime = re.sub(r'[^0-9]', '', commit_date)
version = publish.version.format(datetime=datetime, date=datetime[:8], time=datetime[8:])

and with a provided version v0.{date}.{time} it would give v0.20231106.161132.

Of course the sky's the limit on how much data we want to provide on these templates (e.g., each individual date part on its own to be able to do crazier things v{year}.{month}{day}.{time}), but even just these simple fields are a great starting point and should cover what most people would want to do (and they can always create a custom version with bash scripting before the publish github action)

This might also be nice if in the future we can somehow add other data (e.g., commit count).

Copy link
Member

@domenkozar domenkozar Nov 8, 2023

Choose a reason for hiding this comment

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

I think we should separate version and rolling, which should be the patch part of the semver.

The reason for this is that we want to be able to know what are the different versions that run as rolling (in case of nixpkgs).

So can do something like where version could then default to 0.1.${date} and provide a few formatted options.

- uses: flakestry/flakestry-publish@main
  with:
    ref: "${{ inputs.ref || github.ref }}"
    version: "0.1.${datetime}"

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's two distinct scenario's with subtle variations each. (MAJOR etc are hardcoded by the branch contents. ${n} is handled by flakestry or is a datetime)

  • pre-releases

    • automatic patch releases without tags
      • MAJOR.MINOR.${n}
      • MAJOR.MINOR.PATCH.${n}
    • proper prereleases that bump the version. Many tools recognize that 1.0 > 1.0pre1, as the RPM version comparison is somewhat of an industry standard, and is implemented by Nix.
      • MAJOR.MINORpre-${n}
  • Proper rolling release. Branch contents do not contain any version info. These should not be comparable with real releases. Embedding these into the 0.0. or 0.1. seems like a reasonably safe bet, and makes it so that we don't have to do something overly creative for such releases.

    • datetime of publishing, or
    • counter, or
    • git commit depth aka revCount
    • lastModified, which is more of a workaround for when you don't have the datatime of publishing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, just added a quick change to allow for using datetime in the version.

Now, for the matter of how to interpret versions as pre-releases / rolling, I don't think it's up to us to do much since we can't tell if a tag is missing because it's a pre-release or because it's rolling and versions don't really apply. What I think we need to do is just enable users to publish either easily. By providing the version along the ref, they can define however they want to version these pre-releases. If they provide a ref but no version we can either assume that they don't really care about the version, and just shove it all in a 0.1.X or similar scheme; or we can throw errors forcing them to define their own version for these rolling releases.

For nixpkgs as mentioned above, we can have 23.11.x and 0.1.x, where the former is the stable version nixpkgs-23.11 and the later is the nixpkgs-unstable.

roberth's example also brings another question. Should we be more open to what a "version" looks like? I mean, currently the regex is quite restrictive and only allows numbers, either 2 or 3 segments, and an optional v at the front. No place for pre-release tags at the end, which means that we're not actually following the full semver scheme, and could be a bit annoying for pre-releases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw just remembered that semver.org has a recommended regex, which I modified slightly to allow the preceding v and provide the same capture group

^v?((?:0|[1-9]\d*)\.(?:0|[1-9]\d*)\.(?:0|[1-9]\d*)(?:-(?:(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+(?:[0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?)$

(also, https://regex101.com/r/c5ENND/1)

a bit of a mouthful, but this allows for pre-releases, snapshots, and other such semver semantics. Now, we might just not want to support that, but just throwing it out there in case we do.

@GaetanLepage
Copy link

Hi !
Thank you for this nice work (project and PR).
I would like to publish the nix-community/nixvim project to flakestry.
However, we do not have semantic releases. Is there a trick for the deployment to work in "rolling" mode right now ?
@domenkozar you have previously suggested to use v1.0-<time>. Is that still working ?

@domenkozar
Copy link
Member

I'll be at https://thaigersprint.org/ in a few weeks and I'll merge this and release non-release workflow :)

@kip93
Copy link
Contributor Author

kip93 commented Jan 27, 2024

I've also come back from an extended holiday vacation, and I'm open to any changes that need to be made (:

@domenkozar
Copy link
Member

Something is wrong with formatting: https://staging.flakestry.dev/publish

@domenkozar
Copy link
Member

8cb57ef

@kip93 kip93 deleted the feature/non-tags branch February 18, 2024 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rolling release support
5 participants