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

Script to fetch corresponding branches of dependent projects #3945

Merged
merged 8 commits into from
May 19, 2017

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented May 17, 2017

So we can run the automated tests against the correct branches

dbkr added 4 commits May 17, 2017 16:11
Fetch branches of js-sdk and react-sdk that match the current
branch name, if they exist. This will mostly be used in the
automated tests.
@t3chguy
Copy link
Member

t3chguy commented May 17, 2017

To make this work for org-member PRs you could leverage something like:

if [[ "$TRAVIS" == true ]]; then
	curbranch="${TRAVIS_PULL_REQUEST_BRANCH:-$TRAVIS_BRANCH}"
else
	defbranch=`git rev-parse --abbrev-ref HEAD`
	curbranch="${ghprbSourceBranch:-$defbranch}"
fi

to determine the current branch

So we hopefully get the right branch for PRs from the same repo
(but not forks).

From @t3chguy's comment (tweaked a bit)
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

some stupid questions, but more generally:

The problem with this is that we won't be following our own instructions, which means that we won't know if we've broken things for people following those instructions. I wonder if we should special-case develop, or alternatively, give up on the idea of trying to make package.json right on the develop branch, and tell anyone trying to build develop from source to do the git checkouts themselves.

@@ -2,6 +2,5 @@ language: node_js
node_js:
- 6 # node v6, to match jenkins
install:
- scripts/fetch-develop.deps.sh
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure there was a reason we did npm install on riot-web before trying to symlink the dependencies both here and in jenkins.sh (like: npm shits all over the stuff you've just installed). The README also says you should do it the other way around.

Are you sure it works this way around?

Copy link
Member Author

Choose a reason for hiding this comment

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

It empirically does, although only did it this way to avoid installing copies of thing we then blow away again.

# Look in the many different CI env vars for which branch we're
# building
if [[ "$TRAVIS" == true ]]; then
curbranch="${TRAVIS_PULL_REQUEST_BRANCH:-$TRAVIS_BRANCH}"
Copy link
Member

Choose a reason for hiding this comment

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

this, I suspect, isn't going to work when the PR is from a remote repo. I guess that's ok though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it won't sadly - there was some discussion on this on #riot-web - basically we'd need to do a lot more to guess that, so we're not going to do so now (nothing to stop us from doing it in the future though)

Copy link
Member

Choose a reason for hiding this comment

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

#riot-dev * (incase @richvdh feels like looking at it)

@richvdh richvdh assigned dbkr and unassigned richvdh May 18, 2017
Update README instructions and add checks to release script to
prevent us forgetting to bump the versions of dependencies
(because the check in the main release script will only catch
references to #develop left in, which will no longer be the
failure mode).
@dbkr dbkr assigned richvdh and unassigned dbkr May 19, 2017
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm

@dbkr dbkr merged commit 50b46af into develop May 19, 2017
@t3chguy t3chguy deleted the dbkr/fetch_deps_script branch May 12, 2022 08:57
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.

3 participants