Skip to content

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

Merged
merged 4 commits into from
Sep 2, 2022
Merged

Raise the number of new possible tags #588

merged 4 commits into from
Sep 2, 2022

Conversation

LeoColomb
Copy link
Collaborator

@LeoColomb LeoColomb commented Sep 2, 2022

And make sure they have to be created.
Closes roots/wordpress#44

A little bit "wtf" I'm afraid.

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,
Copy link
Collaborator Author

@LeoColomb LeoColomb Sep 2, 2022

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(
Copy link
Collaborator Author

@LeoColomb LeoColomb Sep 2, 2022

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

)
)
return targetTags
.filter((req) => req.status === 'rejected') // Non existent tags
Copy link
Collaborator Author

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

.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'
Copy link
Collaborator Author

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.

Copy link
Member

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)

Copy link
Collaborator Author

@LeoColomb LeoColomb Sep 2, 2022

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

Copy link
Collaborator Author

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 😁

Copy link
Member

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?

Copy link
Collaborator Author

@LeoColomb LeoColomb Sep 2, 2022

Choose a reason for hiding this comment

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

Octokit rejects promises if fetch/xhr is failing, but inside a Promise.allSettled with then/catch it makes it wrapped into another always fulfilled promise.

image

@LeoColomb LeoColomb enabled auto-merge (squash) September 2, 2022 21:09
@LeoColomb LeoColomb merged commit 9af2049 into main Sep 2, 2022
@LeoColomb LeoColomb deleted the fix/mp-workflow-12 branch September 2, 2022 23:36
LeoColomb added a commit that referenced this pull request May 24, 2023
Follow up of #588
Still to fix roots/wordpress#44

The number of new WordPress releases per announcement is raising to the sky 🌪️
LeoColomb added a commit that referenced this pull request May 24, 2023
Follow up of #588
Still to fix roots/wordpress#44

The number of new WordPress releases per announcement is raising to the sky 🌪️
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.

Bug: New minor WordPress versions not available
2 participants