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

configlet-ci: use track's fetch-configlet script #106

Closed
wants to merge 1 commit into from

Conversation

ee7
Copy link
Member

@ee7 ee7 commented Jan 6, 2023

(Untested).

Can we do something like this?

Refs: #101 (maybe closes?)
Closes: #103

@ee7 ee7 force-pushed the configlet-ci-use-track-fetch branch from 505ee72 to 5f681d4 Compare January 6, 2023 10:30
@ErikSchierboom
Copy link
Member

No idea. We'll have to test. Also, I think this won't have the GITHUB_TOKEN value available in the script.

@ee7
Copy link
Member Author

ee7 commented Jan 6, 2023

Can we do the approach of 882e4a1?

Testing that, it seems to work:

@ErikSchierboom
Copy link
Member

Yes, that works, but I'd be most interested in whether it use the github_token (I expect it doesn't). Could you test that?

@SaschaMann
Copy link
Contributor

SaschaMann commented Jan 6, 2023

It was intentionally not meant to use the track's fetch-configlet script for stability and simplicity (no need for all the OS handling if the environment is known). Once it can be ensured that the track's script exists, isn't broken, and is always up-to-date, this would be a low-risk change, but that's not the case yet afaik.

@ErikSchierboom
Copy link
Member

@SaschaMann
Copy link
Contributor

Merging of org-wide-files syncs isn't automatic, you'd still run into situations where the script might be out of date. For a CI script, that seems like an issue.

@ee7
Copy link
Member Author

ee7 commented Jan 6, 2023

For now, let's close in favor of #107.

One advantage of using gh here is that it's guaranteed to fail if we don't pass the token in correctly. With the curl version, we might only notice that we're making unauthenticated requests when we hit rate limits.

@ee7 ee7 closed this Jan 6, 2023
@ee7 ee7 deleted the configlet-ci-use-track-fetch branch January 6, 2023 16:18
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.

fetch-configlet: DRY/sync with version in configlet repo
3 participants