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

Generalize boot.sh to support running any ref #187

Merged
merged 4 commits into from
Jul 27, 2024

Conversation

rmacklin
Copy link
Contributor

@rmacklin rmacklin commented Jul 4, 2024

This will enable running not only the master and stable branches 1, but also unmerged commits, for easier testing, e.g.

export OMAKUB_REF=2f1544548d8709f61e9e6aac5e222b000770898b
wget -qO- "https://raw.githubusercontent.com/basecamp/omakub/${OMAKUB_REF}/boot.sh" | bash

The hope is this would provide a good way to have folks test things like #29 before they've been merged.

Additionally, together with:

it removes the need for a separate boot-dev.sh script (which was nearly identical to boot.sh).

Footnotes

  1. Note: stable and master will still be runnable via the existing wget -qO- https://omakub.org/install | bash and wget -qO- https://omakub.org/install-dev | bash commands, respectively. No workflow changes needed for those two common cases!

rmacklin added a commit to rmacklin/omakub-site that referenced this pull request Jul 4, 2024
With the changes from basecamp/omakub#187 we can
get by with a single boot script.
@rmacklin rmacklin marked this pull request as draft July 4, 2024 06:37
@rmacklin rmacklin marked this pull request as ready for review July 4, 2024 06:57
This will enable running not only the `master` and `stable` branches,
but also unmerged commits, so that we can easily test them.
Otherwise it'd try to check out the branch it's already on.
@rmacklin
Copy link
Contributor Author

@dhh Curious to get your thoughts on this. I just updated the PR description to clarify that this would not take away the common case of running
wget -qO- https://omakub.org/install | bash or
wget -qO- https://omakub.org/install-dev | bash.

The goal is simply to support a similar mechanism for testing other changes that aren't merged to master or stable (like the work-in-progress changes to add ARM support, as one example where I see this being useful). And a little side-benefit is it obviates the need to maintain a separate, nearly identical boot-dev.sh script.

That said, let me know if you have any objections to this, so I can either try to address them or abort and close the PRs.

@dhh
Copy link
Member

dhh commented Jul 27, 2024

How would you be able to test something that isn't on master through this? Wouldn't those PRs be on a separate cloned repo URL?

@rmacklin
Copy link
Contributor Author

In GitHub, fork repositories share the same underlying git data, i.e. commits from a fork can be viewed/fetched in the base repository. Branches and tags are scoped to individual repositories, but the underlying commits aren't - it's been that way since forks were introduced, and is foundational to the GitHub repository "network". So we're just leveraging that here.

@dhh dhh merged commit e1bf234 into basecamp:master Jul 27, 2024
@rmacklin
Copy link
Contributor Author

rmacklin commented Jul 27, 2024

I should clarify that in order to test a PR like #29 or #227 using this method, the PR would have to be rebased or have master merged into it. Any new PRs branched off master will be good to go.

@rmacklin rmacklin deleted the generalize-boot.sh branch July 27, 2024 16:34
@dhh
Copy link
Member

dhh commented Aug 2, 2024

This is breaking the update script. Since you're no longer directly on the stable branch. Can you have a look at how we might fix this? And also how we might fix it for folks who've installed this in a way where update now won't run?

@rmacklin
Copy link
Contributor Author

rmacklin commented Aug 2, 2024

I've opened #240 which I think should address this going forward. Unfortunately, I think folks who did a first-run install in v1.1.2 would have to manually run cd $OMAKUB_PATH && git checkout stable before updating omakub...

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