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

Fetch script now downloads script to execute #139

Closed
wants to merge 9 commits into from

Conversation

ErikSchierboom
Copy link
Member

@ErikSchierboom ErikSchierboom commented Jan 27, 2021

For the background/motivation, see: #181

These scripts will become  the source of truth and will be downloaded by the new fetch scripts
…ript

With this change, we can now update the actual fetch script used by all tracks in a single location
scripts/fetch-configlet Outdated Show resolved Hide resolved
@ErikSchierboom ErikSchierboom marked this pull request as ready for review January 27, 2021 15:39
@ErikSchierboom
Copy link
Member Author

At some point, it would be good to add a test too to verify that the fetch-configlet script actually works. But I think we can postpone that.

@ee7
Copy link
Member

ee7 commented Jan 28, 2021

At some point, it would be good to add a test too to verify that the fetch-configlet script actually works. But I think we can postpone that.

Indeed. Let's keep track of that in #121

Copy link
Member

@ee7 ee7 left a comment

Choose a reason for hiding this comment

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

Side note: I don't love fetch-configlet-script as a filename, but I dont have a better suggestion right now.

scripts/fetch-configlet Outdated Show resolved Hide resolved
scripts/fetch-configlet Outdated Show resolved Hide resolved
scripts/fetch-configlet Outdated Show resolved Hide resolved
ErikSchierboom and others added 2 commits January 28, 2021 10:55
Co-authored-by: ee7 <45465154+ee7@users.noreply.github.com>
Co-authored-by: ee7 <45465154+ee7@users.noreply.github.com>
@ErikSchierboom
Copy link
Member Author

Side note: I don't love fetch-configlet-script as a filename, but I dont have a better suggestion right now.

Yeah me neither. I'm open for suggestions

@ErikSchierboom
Copy link
Member Author

This PR seems to be controversial after having spoken to some people, so I'll close this PR. The strategy will instead be to automatically send PRs to tracks to update.

@ee7
Copy link
Member

ee7 commented Jan 28, 2021

This PR seems to be controversial after having spoken to some people, so I'll close this PR. The strategy will instead be to automatically send PRs to tracks to update.

Sounds reasonable. I was gradually leaning that way too.

I had this reservation: the "fetch-fetch" approach is only a win overall if the "fetch-fetch" script needs updating less often than the plain fetch script. But the "fetch-fetch" approach is less mature and has had fewer eyes on it, so it's likely to require updates if it has any complexity at all, and some complexity is arguably merited.


rm -f "${output_path}"
# Execute the fetch script
bash -c "${script}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Could exec bash -c "${script}" here to replace the running process. That's (kind of) the shell's "tailcall" mechanism.

Copy link
Member

Choose a reason for hiding this comment

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

Great feedback, thanks. Sorry for the slow reply - still catching up on notifications.

We decided to stick with the fetch-configlet status quo for now, but I imagine we'll use your suggestion if we revisit the "fetch-fetch-configlet" idea.

@glennj
Copy link
Contributor

glennj commented Jan 28, 2021

Side note: I don't love fetch-configlet-script as a filename, but I dont have a better suggestion right now.

Yeah me neither. I'm open for suggestions

configlet-fetcher ?

@ErikSchierboom ErikSchierboom deleted the meta-fetch-script branch January 29, 2021 14:00
@ee7
Copy link
Member

ee7 commented Feb 5, 2021

Side note: I don't love fetch-configlet-script as a filename, but I dont have a better suggestion right now.

Yeah me neither. I'm open for suggestions

configlet-fetcher ?

Well, it doesn't have "script" in the filename, so I like it :)

I guess fetch-fetcher was possible too.

I think the clearest naming was fetch-fetch-configlet and fetch-configlet, but we wanted to keep the repo file named fetch-configlet for backwards-compatibility, even though it'd perform "fetch-fetch".

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.

4 participants