Add PR number to Github instances and show PR link in the Web UI. #4
Conversation
| | <a href="{{ selected.instance.github_pr_url }}" target="_new">PR#{{ selected.instance.github_pr_number }}</a> | ||
| {% endif %} | ||
| </li> | ||
| {% endif %} |
There was a problem hiding this comment.
@smarnach Is that showing up correctly on your development environment? This template isn't interpreted by the Django templating language, it uses the angular syntax, so {% if ... %} won't work - it shows this for me:
Instead you need to use the ng-if directive, like in https://github.com/open-craft/opencraft/blob/smarnach/add-pr-link/instance/static/html/instance/index.html#L33
There was a problem hiding this comment.
Oh dear, apparently I was quite tired on Friday night. :) As mentioned in the PR description, I had some trouble getting the development server running. I'll give it another try and send an update.
EDIT: Argh, I just noticed that there is no PR description, since I only clicked the "Preview" button. Anyway, the development server now works for me and the comments there have become irrelevant.
There was a problem hiding this comment.
@smarnach : )
Out of curiosity, what were the issues you had to setup the dev environment? If there are some gotchas other might run into, it could be worth adding a note or fixing them.
There was a problem hiding this comment.
@antoviaque The problems were mostly with myself, not with the instructions in the README. The README could be a bit more detailed on what to put in the .env file and which settings actually need useful values to get the development server running. (I simply copied .env.test, removed the -test suffix from the database name and ran createdb opencraft, which is probably the fastest way of getting a somewhat working setup.)
There was a problem hiding this comment.
@smarnach Yup, that seem like good instructions to add to the README - starting from the test environment can get things to start. For having everything working, there is not really a way around going through the configuration file, and setting the right values/creating the right accounts. Configuration seem a recurring pain point, you're not the first one to mention it - but not sure how to do much better than this.
|
@antoviaque I pushed a fix for the issues you mentioned. This time I verified it myself, since I now have a working development server. :) |
|
@smarnach Thanks for the fixes! Looks good now to me 👍 |
Add PR number to Github instances and show PR link in the Web UI.
| """ | ||
| if self.github_pr_number is None: | ||
| return None | ||
| return '{0.github_base_url}/pull/{0.github_pr_number}'.format(self) |
There was a problem hiding this comment.
@smarnach I just realized, testing on the real set of sandboxes, that there is a bug here - github_base_url is the base URL of the repo being deployed - not the repo the PR is opened on. This works well when they are the same, but not when the PR is opened from a fork.
This should be using settings.WATCH_FORK or - likely better since we'll eventually watch mutiple forks - store the full PR URL instead of just the number. Since this is already merged and the ticket closed, I'll add a ticket in the backlog about this.
There was a problem hiding this comment.
Oh, darn, of course. Yes, adding the full URL seems like the best solution until we have some more complete PR model.
# 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 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
No description provided.