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

restoring checkout of update branch to be a new branch #41

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vsoch
Copy link
Contributor

@vsoch vsoch commented Mar 12, 2020

previously we had just git checkout -b and this worked because the branch was not fetched on the github actions host - adding the OR to checkout an existing branch seems to have triggered an error. All I am doing here is restoring those lines in both recipes to what they used to be. It's a little confusing looking at the Actions tab because although there was a failure yesterday, the contributors graphic was updated, so perhaps this was the day before? Here is the previously working version of one of the files https://github.com/sourcecred/sourcecred-action/blob/b9e2bdb4fac6f0f809adf35098bdd9220aed6e06/.github/workflows/contributors_automated.yml#L25. If it triggered again, we might want to grab changes before we run the containers.

Signed-off-by: vsoch [email protected]

previously we had just git checkout -b <branch> and this
worked because the branch was not fetched on the github
actions host - adding the OR to checkout an existing branch
seems to have triggered an error. All I am doing here
is restoring those lines in both recipes to what they used
to be.

Signed-off-by: vsoch <[email protected]>
@vsoch
Copy link
Contributor Author

vsoch commented Mar 12, 2020

@Beanow I think we also need a pull from master since we start with it and it wasn’t up to date on the other branch, let me know your thoughts!

@Beanow Beanow self-requested a review March 12, 2020 17:10
@vsoch
Copy link
Contributor Author

vsoch commented Mar 13, 2020

@Beanow I'm not sure that we need this anymore - the contributors graphic ran okay last night, no issues! If you agree I can close this here.

@vsoch vsoch closed this Mar 13, 2020
@Beanow
Copy link

Beanow commented Mar 19, 2020

The answer isn't immediately obvious to me here, so I agree to close it until it proves to be an actual problem :]

@vsoch
Copy link
Contributor Author

vsoch commented Mar 20, 2020

@vsoch vsoch reopened this Mar 20, 2020
Copy link

@Beanow Beanow left a comment

Choose a reason for hiding this comment

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

So it seems to me like there's a more fundamental issue here.
We're first generating new data, like contributors/prototype.

Then we're trying to check out the latest remote changes. Regardless of what the exact git command looks like here, I would expect there to be a failure because the local file we just generated would be overwritten by the remote branch we're trying to check out.

This isn't an issue if we don't care about the history and want to force-push a commit. But if we want to preserve history, you'll probably need either: checkout first, then generate new files, then commit-push.
Or generate, stash changes, checkout, pop stash (hope there's no conflicts), commit-push.

@@ -34,13 +34,13 @@ jobs:
git remote set-url origin "https://x-access-token:${GITHUB_TOKEN}@github.com/${GITHUB_REPOSITORY}.git"
git branch
printf "Branch to push to is ${UPDATE_BRANCH}\n"
git checkout -b ${UPDATE_BRANCH} || git checkout ${UPDATE_BRANCH}
git checkout -b "${UPDATE_BRANCH}"
Copy link

Choose a reason for hiding this comment

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

I notice this is the only place with quotes for the branch. With quotes seems like the right way to go about it, let's apply that to other instances (at least for changed lines in this PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I paused working on this because we’d replace with cred-action anyway.

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