fix(github): Fix making github releases latest or not #536
Conversation
|
can you describe the issue? it's unclear what you're trying to solve for (additionally this PR makes changes beyond logging) |
The problem is that it does not seem to be working, it is still marking the stuff as latest. But I can't see anything wrong, and the existing logs make it impossible to know where this failed (e.g. could it not find the previous release? is the version from there picked up incorrectly? ...) |
|
probably don't need the logging -- the problem is that the draft was created correctly (non-latest) but then converted to latest when moved out of draft because this defaults to |
|
I think the additional logging makes sense but it's better not to change other stuff. |
Ahh, great catch. I'll update this accordingly! I think I'll leave the logging in, it can still be helpful I'd say? At least the log for what the previous release was! |
🤷 personally I'd rather not just because it's even more output clutter -- and it wouldn't have helped us find the answer here |
mydea
changed the title
|
So I updated this to ensure we set |
|
as I requested above, and as I've done in many of your PRs: please limit the change to just the functional change instead of coupling it with unrelated (and imo undesirable) changes. this makes it easier to review, easier to reason about, easier to rollback, and generally is how one should approach development |
mydea
force-pushed
the
fn/better-logging-github-latest
branch
2 times, most recently
from
60113da to
87fca20
Compare
|
I updated this to only include minimal changes necessary to make this work. The core change is that we ensure to also set |
There was a problem hiding this comment.
Requesting changes not because of the code but for the lack of a PR description explaining what was going wrong, what exactly we are fixing etc.
If this is an important change and if it's not too hard, adding a test would also be nice (I'm fine not adding if there are none though)
src/targets/github.ts
Outdated
| @@ -345,6 +337,7 @@ export class GitHubTarget extends BaseTarget { | |||
| await this.github.repos.updateRelease({ | |||
| ...this.githubConfig, | |||
| release_id: release.id, | |||
| make_latest: isLatest ? 'true' : 'false', | |||
There was a problem hiding this comment.
Here: https://docs.github.com/en/rest/releases/releases?apiVersion=2022-11-28#create-a-release it is documented as type string, for whatever reason...
src/targets/github.ts
Outdated
| @@ -111,7 +111,8 @@ export class GitHubTarget extends BaseTarget { | |||
| public async createDraftRelease( | |||
| version: string, | |||
| revision: string, | |||
| changes?: Changeset | |||
| changes?: Changeset, | |||
| isLatest = true | |||
There was a problem hiding this comment.
| isLatest = true | |
| setAsLatest = true |
Also, not to derail your patch but boolean function arguments are frowned upon as they make it very hard to see what's going on from the callsite: createDraftRelease('1.2.3', 'aaaef32', undefined, false) would be quite hard to make sense of.
You may want to consider converting all of them to an arguments object or just add a final options object with setAsLatest as its only known key for now?
There was a problem hiding this comment.
happy to do that, I tried to do minimal changes in the PR, but I agree - I'll convert this to an object!
src/targets/github.ts
Outdated
| @@ -336,7 +328,7 @@ export class GitHubTarget extends BaseTarget { | |||
| * | |||
| * @param release Release object | |||
| */ | |||
| public async publishRelease(release: GitHubRelease) { | |||
| public async publishRelease(release: GitHubRelease, isLatest = true) { | |||
There was a problem hiding this comment.
Same concern as above re boolean args and also naming. Would prefer something like setAsLatest or makeLatest
mydea
force-pushed
the
fn/better-logging-github-latest
branch
from
87fca20 to
f5d226d
Compare
This PR updates the github release creation to ensure that the release is correctly marked as latest.
We used to set this for when we created the release draft, but according to https://docs.github.com/en/rest/releases/releases?apiVersion=2022-11-28#create-a-release, this cannot actually be set on drafts. Instead, we need to set it when we publish the release.
There is also an option
legacy, which, according to the docs, seems like it may also work:But since this option is a legacy option, and since the behavior is a bit under-specced (e.g. if it determines based on creation date, it may be wrong, if it always uses semantic version, it may be correct), we decided to implement this consistently for ourselves.