-
-
Notifications
You must be signed in to change notification settings - Fork 8
Raise the number of new possible tags #588
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
Conversation
And make sure they have to be created
@@ -38,18 +38,34 @@ jobs: | |||
github.rest.repos.listTags({ | |||
owner: context.repo.owner, | |||
repo: PACKAGE.substring(PACKAGE.indexOf('/') + 1), | |||
per_page: 12, | |||
per_page: 50, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to 50 new versions in a batch (hopefully enough)
return upstreamTags | ||
.filter((tag) => !currrentTags.find((t) => t.name === tag.name)) | ||
.map((tag) => tag.name) | ||
const targetTags = await Promise.allSettled( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An array of already-existing-tags-on-meta-package requests is run to ensure correctness of the workflow
.github/workflows/meta-package.yml
Outdated
) | ||
) | ||
return targetTags | ||
.filter((req) => req.status === 'rejected') // Non existent tags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the request is 404, the tag does not exist and ready to be created
.github/workflows/meta-package.yml
Outdated
.filter((req) => req.status === 'rejected') // Non existent tags | ||
.map((res) => | ||
res.reason.request.url.substring( | ||
res.reason.request.url.indexOf('%2F') + 3 // 'refs/tags%2Fx.y.z' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Retrieve the tag from the request.
Definitely ugly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 what about using the tag we already have above? We just need to wrap that promise above somehow?
This might work:
const targetTags = await Promise.allSettled(
upstreamTags
.filter((tag) => !currrentTags.find((t) => t.name === tag.name))
.map((tag) =>
github.rest.git.getRef({
owner: context.repo.owner,
repo: META.substring(META.indexOf('/') + 1),
ref: 'tags/' + tag.name,
}).then(res => ({ tag: tag, res }))
)
)
return targetTags
.filter(({res}) => res.reason.status === 404) // Non existent tags
.map({tag} => tag.name)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's smart, not that obvious though:
- The
then
is not sufficient as we need to cover the failure as well (actually the failure is the value here) Promise.allSettled
has a weird behavior to always wrap the results into.value
const targetTags = await Promise.allSettled(
upstreamTags
.filter((tag) => !currrentTags.find((t) => t.name === tag.name))
.map((tag) =>
github.rest.git
.getRef({
owner: context.repo.owner,
repo: META.substring(META.indexOf('/') + 1),
ref: 'tags/' + tag.name,
})
.then(() => {})
.catch((res) => ({ tag: tag.name, status: res.status }))
)
)
return targetTags
.filter(({ value }) => value?.status === 404)
.map(({ value }) => value.tag)
PR updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that's still a bit wtf 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my brief check, octokit never rejects a promise. It's always fulfilled as a success just with an error message in it. Did you see otherwise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow up of #588 Still to fix roots/wordpress#44 The number of new WordPress releases per announcement is raising to the sky 🌪️
Follow up of #588 Still to fix roots/wordpress#44 The number of new WordPress releases per announcement is raising to the sky 🌪️
And make sure they have to be created.
Closes roots/wordpress#44
A little bit "wtf" I'm afraid.