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

Rename checkver_token to gh_token and SCOOP_CHECKVER_TOKEN to SCOOP_GH_TOKEN #4832

Merged
merged 2 commits into from Mar 21, 2022
Merged

Conversation

ghost
Copy link

@ghost ghost commented Mar 21, 2022

Checklist:

  • I have read the Contributing Guide.
  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.

@rashil2000
Copy link
Member

rashil2000 commented Mar 21, 2022

Additionally, can you replace all instances of checkver_token with gh_token (4 in total) in this PR?

And add an entry in the changelog

@ghost ghost changed the title Properly filtering null input in dl function when private_hosts is not set in config Rename checkver_token to gh_token Mar 21, 2022
@ghost ghost changed the title Rename checkver_token to gh_token Rename checkver_token to gh_token and SCOOP_CHECKVER_TOKEN to SCOOP_GH_TOKEN Mar 21, 2022
@ghost
Copy link
Author

ghost commented Mar 21, 2022

@rashil2000 done, please review

@rashil2000
Copy link
Member

https://github.com/ScoopInstaller/GithubActions/blob/bd73d5711c4ad072c73e17d41b592e57d71892c6/src/Action/Scheduled.psm1#L24

This also needs to be updated then, I'll do it once this is merged

@rashil2000 rashil2000 merged commit ad3fc4f into ScoopInstaller:develop Mar 21, 2022
@ghost ghost deleted the private-repos-fix branch March 21, 2022 22:00
rashil2000 added a commit to ScoopInstaller/GithubActions that referenced this pull request Mar 21, 2022
@rashil2000
Copy link
Member

https://github.com/ScoopInstaller/GithubActions/blob/bd73d5711c4ad072c73e17d41b592e57d71892c6/src/Action/Scheduled.psm1#L24

This also needs to be updated then, I'll do it once this is merged

This can't be done yet. We will have to wait for a new release.

@niheaven
Copy link
Member

This tends to be a refactor, not a feature, will change the CHANGELOG next time.

@ghost
Copy link
Author

ghost commented Mar 22, 2022

This tends to be a refactor, not a feature, will change the CHANGELOG next time.

I was debating between these two and decided to go with feature since this change affects the users (env var change, configuration parameter change). However if refactoring in the sense you use it allows "breaking" changes, I agree this is refactoring.

@niheaven
Copy link
Member

niheaven commented Mar 22, 2022

Yes, adding a "breaking change" at the top of release notes for this one is a good idea, since it is UX change, right? 😄

@ghost
Copy link
Author

ghost commented Mar 22, 2022

Yes, adding a "breaking change" at the top of release notes for this one is a good idea, since it is UX change, right? 😄

Exactly. I searched in the changelog for something like this, but didn't find...

@niheaven
Copy link
Member

Exactly. I searched in the changelog for something like this, but didn't find...

Because we haven't had such BREAKING CHANGES since we have changelog...

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.

2 participants