Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
One thing to note is that if code is pushed via non-git means, then the fetched branch doesn't reflect the actual commit that is deployed. I've seen this before on staging with deploys done by the ops bot. On production that shouldn't be an issue, but on the rare occasion that we rollback a deploy, the remote repo state and deployed state will diverge.
Also, I wonder if we should parse
heroku-prod/HEAD
, as at some point we may switch to amain
branch which Heroku now supports.Alternatively, we could extract the deployed commit from the server by fetching
https://crates.io/api/v1/site_metadata
. This should always reflect the latest deployed commit hash.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.
does that bot not use git to deploy?
let me try if that works, but generally sounds reasonable
seems like that would require something like https://stedolan.github.io/jq/ and I was hoping to avoid external dependencies for this script to keep it simple :)
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.
unfortunately it doesn't :-/
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'm not too familiar with that codebase, but this looks like the relevant logic. I believe it makes an API request to Heroku which then fetches from GitHub. From what I recall, that method doesn't update the master branch of Heroku's version of the repo.
It looks like
git remote set-head heroku-prod -a
would lookup the remoteHEAD
and cache it asheroku-prod/HEAD
, however I think that is a one-time operation and would need to either be run every time by the script, or every user would have to remember to run it if we change to a differentHEAD
branch. Therefore, it seems fine to me to just referencemaster
directly and update the script if we ever rename the branch tomain
.I agree that it doesn't seem worth adding a dev dependency for this. Also, given that there are only a few of us who will run this script, I think we can remember to tweak the generated URL as necessary if we're end up rolling back a deployment.
This looks good to me, but I'll defer to @pietroalbini for the final r+.
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 see, thanks :)