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 latest release #269

Merged
merged 2 commits into from
Jun 15, 2024

Conversation

Schultzer
Copy link
Contributor

@Schultzer Schultzer commented May 12, 2024

Description

This PR closes #266 and supersedes #267; where I got a bit lost in the codebase.

Closes #266

@Schultzer Schultzer mentioned this pull request May 12, 2024
2 tasks
@paulo-ferraz-oliveira
Copy link
Collaborator

Haven't read the code yet, but I had an unanswered question in the other pull request.

Let's say erlang/otp releases 24.2.5, but 24.2.4 is the one set to "latest". Shall a rule in setup-beam override that? Or where will the semantics reside?

@paulo-ferraz-oliveira
Copy link
Collaborator

I have read and understood the contributing guidelines

This states "Execute npm run build-dist and fix any issues arising from that". I can't understand what failed that it didn't generate any artefacts that you'd need to push, for which CI complained later.

@Schultzer
Copy link
Contributor Author

Haven't read the code yet, but I had an unanswered question in the other pull request.

Let's say erlang/otp releases 24.2.5, but 24.2.4 is the one set to "latest". Shall a rule in setup-beam override that? Or where will the semantics reside?

Yeah, so if we want to provide a more true latest release then we need to do the reconciliation which would require a bigger refactoring, I tried walking down that path in the other PR but decided to bail out.

If there is a big interest in this we can certainly do it and we can leverage semver to do the heavy lifting by sorting the versions.

Regarding OTP and it’s branch versioning scheme, that can be treated as SemVer build meta and be sorted accordingly.

In the contribution guide it’s mentioned to keep changes to a minimum, so as a new contributor then I’m hesitant to rip apart most of this as it’s currently stand.

@Schultzer Schultzer force-pushed the add-latest-version branch 2 times, most recently from 3f89461 to 5d80cab Compare May 16, 2024 16:47
@Schultzer
Copy link
Contributor Author

I have read and understood the contributing guidelines

This states "Execute npm run build-dist and fix any issues arising from that". I can't understand what failed that it didn't generate any artefacts that you'd need to push, for which CI complained later.

I don't know how that didn't get pushed the first round, but I've rebased and regenerated the dist. Hopefully, it will pass this time around.

@paulo-ferraz-oliveira
Copy link
Collaborator

In the contribution guide it’s mentioned to keep changes to a minimum, so as a new contributor then I’m hesitant to rip apart most of this as it’s currently stand.

I understand, and thank you for being patient and walking us through your thought process. The changes mostly seem sane to me (it's touching parts of the code that are somewhat sensitive), but I think I need other reviewers to pitch in.

In any case, tests are failing and it's probably due to latest being a moving target. You'll probably want to do static tests against a specific input list (and not reading from remote, as that'll surely fail as OTP keeps releasing patches and other versions on a given major branch).

@Schultzer
Copy link
Contributor Author

Schultzer commented May 24, 2024

In the contribution guide it’s mentioned to keep changes to a minimum, so as a new contributor then I’m hesitant to rip apart most of this as it’s currently stand.

I understand, and thank you for being patient and walking us through your thought process. The changes mostly seem sane to me (it's touching parts of the code that are somewhat sensitive), but I think I need other reviewers to pitch in.

In any case, tests are failing and it's probably due to latest being a moving target. You'll probably want to do static tests against a specific input list (and not reading from remote, as that'll surely fail as OTP keeps releasing patches and other versions on a given major branch).

Thanks, I've future-proofed the tests.

@Schultzer Schultzer force-pushed the add-latest-version branch 3 times, most recently from c50ab82 to 809f03c Compare June 12, 2024 03:10
@paulo-ferraz-oliveira
Copy link
Collaborator

Failing tests don't seem to be your responsibility; it's just that rebar3 nightly stopped supporting OTP 24; I'll update that in a separate pull request.

@paulo-ferraz-oliveira
Copy link
Collaborator

@Schultzer, it'd be important to mention this in the README, with all the discussed constraints. Basically:

  1. mention that latest is available
  2. mention that it's calculated locally by the action, not directly related to what's set as latest in the remote repositories
  3. if there's any constraints regarding x.y.z.a.b.c versions in Erlang/OTP also mention those as an exception

@Schultzer Schultzer force-pushed the add-latest-version branch from 809f03c to 02f2471 Compare June 12, 2024 14:04
@paulo-ferraz-oliveira
Copy link
Collaborator

When you squash your changes midway through a review it makes it harder on the reviewer to follow up 😕, since all the changes will now have to be reviewed again... No biggie, just letting you know how I feel it.

@Schultzer Schultzer force-pushed the add-latest-version branch 3 times, most recently from b074fa1 to c26af22 Compare June 12, 2024 22:53
@Schultzer
Copy link
Contributor Author

@Schultzer, it'd be important to mention this in the README, with all the discussed constraints. Basically:

  1. mention that latest is available
  2. mention that it's calculated locally by the action, not directly related to what's set as latest in the remote repositories
  3. if there's any constraints regarding x.y.z.a.b.c versions in Erlang/OTP also mention those as an exception

Done, no known constraints, should be all cake from here. 🍰

@Schultzer
Copy link
Contributor Author

Schultzer commented Jun 12, 2024

When you squash your changes midway through a review it makes it harder on the reviewer to follow up 😕, since all the changes will now have to be reviewed again... No biggie, just letting you know how I feel it.

I apologize, the whitespace bit me, I couldn’t find the commit that had it and I fat fingered git. Which ended up with me squeezing the changes. I’m a complete noob with regards to git.

@paulo-ferraz-oliveira
Copy link
Collaborator

@Schultzer, I don't think we have any more planned changes. I'm thus rebasing this onto the main branch to check if the error issues go away. If they do we can expect a squash+merge soon.

@paulo-ferraz-oliveira
Copy link
Collaborator

This is Ok ✅. I'm squash+merging. You can expect this change in the next release (we want to wait until #278 is done, so we can bump minor). Feel free to come back to this and ask for the release if it's taking more than, say, a week.

@paulo-ferraz-oliveira paulo-ferraz-oliveira merged commit 6ed3c52 into erlef:main Jun 15, 2024
65 checks passed
@Schultzer Schultzer deleted the add-latest-version branch July 13, 2024 16:54
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.

Support latest version tag
2 participants