Skip to content
This repository has been archived by the owner on Aug 22, 2022. It is now read-only.

Full branch url #8

Merged
merged 1 commit into from
Oct 7, 2015
Merged

Full branch url #8

merged 1 commit into from
Oct 7, 2015

Conversation

Kelketek
Copy link
Member

Stores the full URL of a pull request in the instance model instead of just the number.

Also tweaks the makefile to fix issues I ran into when trying to use it.

@Kelketek Kelketek force-pushed the full_branch_url branch 3 times, most recently from ad087be to 6d65fbe Compare September 22, 2015 16:13
@Kelketek
Copy link
Member Author

@antoviaque This is ready for review.

find -name '__pycache__' -type d -delete
find . -name '*.pyc' -delete
find . -name '*~' -delete
find . -name '__pycache__' -type d -delete
Copy link
Member

Choose a reason for hiding this comment

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

@Kelketek Why do we need the . here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just happened to see this comment – for some non-GNU versions of find, the path is mandatory, and it looks like OSX ships with such a version of find.

Copy link
Member

Choose a reason for hiding this comment

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

I see - does that mean that if we include this change, the rest can work natively on MacOS? I'm happy to include it regardless, but for the future I wonder if it's worth the effort to support multiple environments to run it, especially now that we have Vagrant integration.

Copy link
Member Author

Choose a reason for hiding this comment

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

@antoviaque There's actually more changes that need doing to make it work properly on Mac OS X. I was just confused on how this ever worked, since I didn't think this syntax worked in Linux either.

Where's the vagrant info? I'd be happy to drop this change and just use that instead.

Copy link
Member

Choose a reason for hiding this comment

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

@Kelketek You seem to have dropped it already so you might have found it in the meantime, but the latest master has the Vagrantfile: https://github.com/open-craft/opencraft/blob/master/Vagrantfile (cf the README also).

@Kelketek
Copy link
Member Author

Kelketek commented Oct 5, 2015

@antoviaque This is ready for a second look.

@antoviaque
Copy link
Member

@Kelketek Thanks for the changes! LGTM - once we have resolved #8 (diff) I'll give it a try locally to give you the +1.

@antoviaque
Copy link
Member

@Kelketek Thanks for checking, and for the changes! It worked well when I tested locally, both with preexisting cross-fork PRs, and new ones. The only issue I had was that my vagrant VM ran out of memory when trying to run make run, so I posted #17

This will need a rebase, but otherwise LGTM. 👍

Kelketek added a commit that referenced this pull request Oct 7, 2015
@Kelketek Kelketek merged commit af74811 into master Oct 7, 2015
@Kelketek Kelketek deleted the full_branch_url branch October 7, 2015 18:02
"""
Construct the URL for the pull request
"""
return 'https://github.com/{fork_name}/pull/{number}'.format(fork_name=self.fork_name, number=self.number)
Copy link
Member

Choose a reason for hiding this comment

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

@Kelketek I pushed this to production, and this actually still seem wrong - the migrations do the right thing, so this was hiding the issue when I tested locally, but this doesnt, cf

pr_fork_name = r_pr['head']['repo']['full_name']
- fork_name is again the repo from which the PR is sent, not the destination repo of the PR.

xirdneh added a commit that referenced this pull request Jun 30, 2019
# This is the 1st commit message:

Re-writing circle.yml to store code coverage in CircleCI's workspace and
collect them afterwards. Test run parallelization is used with
environment variables instead of using custom test runner.

# This is the commit message #2:

Update Makefile message.

# This is the commit message #3:

Remove cov.html from cov.combine.

# This is the commit message #4:

Stop running all tests in all containers

# This is the commit message #5:

Cleanup

# This is the commit message #6:

Experiment with different IDs for each coverage report

# This is the commit message #7:

Gathering raw coverage from all steps

# This is the commit message #8:

Always copy coverage and log message when not found

# This is the commit message #9:

Log copying raw coverage files for debugging
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants