Closed Schultzer closed 2 months ago
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?
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.
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 insetup-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.
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.
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).
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.
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.
@Schultzer, it'd be important to mention this in the README, with all the discussed constraints. Basically:
latest
is availablelatest
in the remote repositoriesWhen 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, it'd be important to mention this in the README, with all the discussed constraints. Basically:
- mention that
latest
is available- mention that it's calculated locally by the action, not directly related to what's set as
latest
in the remote repositories- 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. 🍰
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.
@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.
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.
Description
This PR closes #266 and supersedes #267; where I got a bit lost in the codebase.
Closes #266