-
Notifications
You must be signed in to change notification settings - Fork 39
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
Accommodate rapid website development on the website
branch
#287
Conversation
@@ -2,7 +2,7 @@ name: Update `error-prone.picnic.tech` website content | |||
on: | |||
pull_request: | |||
push: | |||
branches: [ master ] | |||
branches: [ master, develop ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need develop
? That branch didn't exist before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Below we're mentioning website
, so perhaps it should be s/develop/website/
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, not sure why I typed develop
😅. Pushed the tweak.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can omit this, as long as you open a draft PR. Would that work? Or is there a strong preference to keep using a branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now it would be nice to be able to quickly push something to that branch, such that it'll build. Making draft PRs is a bit more cumbersome. We can revert this after we have everything stable and merged the way we want it.
For now it'll allow us to iterate quickly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, we don't even need to merge this PR. Just push this change on the website
branch, and it'll work :)
@@ -35,7 +35,7 @@ jobs: | |||
with: | |||
path: ./website/_site | |||
deploy: | |||
if: github.ref == 'refs/heads/master' | |||
if: github.ref == 'refs/heads/master' || github.ref == 'refs/heads/website' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also requires a change in our environment settings to allow this branch to deploy.
I would have expected this PR to be just Assuming that's the intention, a suggested commit message could be:
|
I understood something slightly different. Anyway, updated the PR. |
@@ -2,7 +2,7 @@ name: Update `error-prone.picnic.tech` website content | |||
on: | |||
pull_request: | |||
push: | |||
branches: [ master, website ] | |||
branches: [ website ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, if we wish to run the build
job still on master, it should be added to the branches here. This will not overwrite the website as the deploy
job does not run on master.
branches: [ website ] | |
branches: [ master, website ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that would be nice actually, warns us whenever there is an error there. As long as it doesn't override the website for every new merge :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on my understanding, I assume this'll work as intended. 👍
@rickie will add one more commit "soon" 👀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a small commit. Arguably we should also have an XXX comment to drop the website
references later, but let's roll.
Suggested commit message:
Accommodate rapid website development on the `website` branch (#287)
By (a) deploying from that branch and (b) temporarily disabling external link
checking.
error-prone.picnic.tech
deployments from the website
branchwebsite
branch
No description provided.