Skip to content

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
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

3 participants