Skip to content
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

fix: compare only <owner>/<repo> when checking for rename #886

Merged
merged 18 commits into from
Aug 14, 2024

Conversation

jedwards1211
Copy link
Contributor

@jedwards1211 jedwards1211 commented Jul 25, 2024

This PR re-attempts the implementation made in #878 and reverted in #887, by staging a check in the plugin verifyConditions lifecycle using the owner/repo as opposed to the initial implementation where the context.options.repositoryUrl is compared against the fetched repo clone_url.

Related to #885

Screencast

Error Demo

screencast-bpconcjcammlapcogcnnelfmaeghhagj-2024.1.mp4

Success Demo

screencast-bpconcjcammlapcogcnnelfmaeghhagj-2024.2.mp4

@jedwards1211 jedwards1211 changed the title fix: ignore protocol mismatch when checking if repository is renamed fix: compare only <owner>/<repo> when checking for rename Jul 25, 2024
lib/verify.js Outdated Show resolved Hide resolved
lib/verify.js Outdated Show resolved Hide resolved
Copy link
Member

@babblebey babblebey left a comment

Choose a reason for hiding this comment

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

Great stuff @jedwards1211,

Do rake a look at the bit of nits and kindly resolve the conflict for now... We are looking to add some tests too... We'll look at how to go about that after this.

@jedwards1211
Copy link
Contributor Author

jedwards1211 commented Jul 31, 2024

@babblebey okay I made those changes and included the current repositoryUrl and git_url values in the error message.

There are unrelated formatting changes in here now that I ran lint:prettier:fix on lib/definitions/errors.js -- is your CI not enforcing the code formatting, or did the formatting settings change?

@jedwards1211
Copy link
Contributor Author

Okay I opened this issue: #890

@babblebey
Copy link
Member

Run npm run lint:prettier:fix

@jedwards1211
Copy link
Contributor Author

sorry for the noise, vscode-prettier's fault

@jedwards1211
Copy link
Contributor Author

jedwards1211 commented Jul 31, 2024

@babblebey okay I added tests for EMISMATCHGITHUBURL. I also noticed that the repo rename check was being skipped if running in a GitHub action, and that it was making a redundant request for the repo information. I was able to move things around to eliminate the redundant request, and check for rename even if running in a GitHub action.

test/integration.test.js Outdated Show resolved Hide resolved
@jedwards1211
Copy link
Contributor Author

Nice testing setup y'all! I like the thoroughness of the GitHub mocking, and also the fact that you aren't using Jest lol

@jedwards1211
Copy link
Contributor Author

@babblebey anything else y'all need on this?

test/success.test.js Outdated Show resolved Hide resolved
Copy link
Member

@babblebey babblebey left a comment

Choose a reason for hiding this comment

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

Hey @jedwards1211 😁,

I gave this a whirl 😉, changes looks great 👍.... and thank you for sticking to this. I have also gone on and written a description for the PR.

In order to fulfil the test coverage requirement mentioned in the reverted PR #887. Please take a look at the review comment.

@babblebey
Copy link
Member

This is great stuff right here @jedwards1211. I commend you for sticking to this yea.

I have given another whirl and added a video demo of my testing to the PR Description (reviewers like that stuff 😉).

@babblebey babblebey requested a review from a team August 5, 2024 20:08
@jedwards1211
Copy link
Contributor Author

nice, thank you!

Copy link
Member

@babblebey babblebey left a comment

Choose a reason for hiding this comment

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

🚀

@babblebey babblebey merged commit 24ea2ee into semantic-release:master Aug 14, 2024
6 checks passed
Copy link

🎉 This issue has been resolved in version 10.1.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants