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

Version 0.25.0: Rework CI and Contributions #90

Merged
merged 48 commits into from
Jun 4, 2022

Conversation

Samasaur1
Copy link
Owner

@Samasaur1 Samasaur1 commented Oct 31, 2020

Here's what's new:

  • Improves PR template & guidelines
  • Runs GitHub Actions testing on all branches & PRs
  • Make Travis run documentation deployment (on tags) and Danger (on PRs)
  • Danger checks that the version is updated, and that there is a reminder to run release.sh
    • release.sh may not be necessary, as the goal of this PR is to remove the development branch entirely.

Here's what has to happen:
- [ ] Ensure the changelog has everything new that is being added
- [ ] Bump version (run updateVersion.sh)

  1. Wait for CI
  2. Merge
    1. Run release.sh

Samasaur1 and others added 6 commits October 17, 2020 18:14
tests are now run on GitHub Actions, for PRs and pushes
Travis CI now runs only Danger and documentation deployment
I don't know why, but this is taking like 15 minutes and counting to install
@Samasaur
Copy link

Samasaur commented Oct 31, 2020

Fails
🚫

Task 1 incomplete: Ensure the changelog has everything new that is being added

🚫

Task 2 incomplete: Bump version (run updateVersion.sh)

🚫 The CHANGELOG was not updated! (put your changes in the UPCOMING section)
🚫 There is no CHANGELOG entry for this version!
🚫 PR title specifies version 0.25.0, while .jazzy.yaml specifies 0.24.1
🚫 Version was not updated in .jazzy.yaml!
🚫

.jazzy.yaml#L5 - Version was not updated in .jazzy.yaml!

Messages
📖 The new version is v0.25.0
📖

Pull request to master for new version detected

module_version: 0.25.0

Generated by 🚫 Danger Swift against 4ab8a1e

Samasaur1 and others added 8 commits October 31, 2020 13:30
The idea is that all PRs will be to master, but that
only some PRs will be a 'new version'.
It probably won't work, but I should keep trying.
I tried to figure out how danger/swift and danger/js interact,
and I think I more or less got it, but unless this works I think
I will need to make a feature request
@Samasaur1
Copy link
Owner Author

Here's the problem I've got: I want Danger to report both as a status check, and as a comment, with inline comments/fails/suggestion support. I also want Danger to run on PRs from forks, which mean I need to expose an API Token for either my account or @Samasaur, my bot account, which means that it could be extracted, which means I need to be careful about its allowed scopes.

The two potential scopes to use are (from this list):

  • public_repo
  • repo:status

public_repo allows commenting, but its problems are:

  • it only lets you report status checks / do inline comments on repos the account has write access to, but if you give the account write access to the repo, it can change the code (and because it is made public so that it can work with forks, this is unacceptable)

repo:status allows reporting status without code access, even when the account has write access to the repo, but:

  • it doesn't seem to be able to comment
  • it needs write access to the repo, so cannot be combined with a public_repo token.
  • doesn't seem to allow inline comments/suggestions/fails

I think ideally there'd be a comment scope so I could create a token with repo:status and comment scope, give the account write access to the repo, and use that. But since that doesn't exist, I'm not sure what to do.

@Samasaur1
Copy link
Owner Author

It might be possible to do what I have listed above using two bot accounts, but at this point I don't care. And Travis CI has now stopped supporting open-source CI (I think?) so I have to deal with that too

@Samasaur1
Copy link
Owner Author

Really I just have to choose between comments without status checks and status checks without comments. Either one will work, since I trust myself to see read comments before merging, and status checks will force me to wait for them. Actually, I'll probably go with status checks, just in case I miss the comment. I think Danger should let me get fairly granular and specific with the status checks, which is nice.

@Samasaur1
Copy link
Owner Author

I was hoping to remove Danger completely, since it's kinda a pain, but c'est la vie

@Samasaur1
Copy link
Owner Author

with GitHub Actions maybe I can run Danger for status checks as myself and leave comments as my bot user?

@github-actions
Copy link

github-actions bot commented Jun 3, 2022

Messages
📖

Pull request to master for new version detected

📖 The new version is v0.25.0
📖 .jazzy.yaml was updated
📖 PR title and .jazzy.yaml agree
📖 There is a CHANGELOG entry for this version
📖 The CHANGELOG entry heading is correct
📖

Upcoming link was updated to show changes between latest version and master

📖 Link line compares to the new version
📖 The CHANGELOG was updated
📖 PR has no tasks (are you sure?)

Generated by 🚫 Danger Swift against f1a42eb

@Samasaur1
Copy link
Owner Author

I also need to fix Danger again, because the tasks are output in the wrong order

@Samasaur1
Copy link
Owner Author

And I don't know what it's complaining about with the two link comments, because I'm fairly sure they're right — I'll double-check

@Samasaur1
Copy link
Owner Author

I'm also still seeing

Response: {
[38](https://github.com/Samasaur1/DiceKit/runs/6732668468?check_suite_focus=true#step:4:39)
  "message": "Resource not accessible by integration",
[39](https://github.com/Samasaur1/DiceKit/runs/6732668468?check_suite_focus=true#step:4:40)
  "documentation_url": "https://docs.github.com/rest/reference/users#get-the-authenticated-user"
[40](https://github.com/Samasaur1/DiceKit/runs/6732668468?check_suite_focus=true#step:4:41)
}

in the Danger logs, which I believe is due to Danger running in the PR and therefore the token not having write access. For now, it's able to comment because the PR isn't coming from a fork, but I probably want to change this to use the pull_request_target event (and use the Dangerfile from my repo, so that the token can't be exploited that way).. See here: https://github.saobby.my.eu.orgmunity/t/github-actions-are-severely-limited-on-prs/18179/20

@Samasaur1
Copy link
Owner Author

And I don't know what it's complaining about with the two link comments, because I'm fairly sure they're right — I'll double-check

OK, well, I was wrong. I had the compare backwards.

@Samasaur1
Copy link
Owner Author

The other link complaint is because I have the CHANGELOG comparing against development instead of master. This is a more conceptual problem. Before this PR, everything was first merged into development and then only merged into master, from development, after that. The important idea about doing it this way was that every commit to master was a version.

The new idea is that there will be no development branch. Everything will be merged directly into master, and when enough stuff is there to warrant a new version, then we tag it (and now docs and releases will be done automatically after that).

So what Danger is saying (i.e., what past-me who set up Danger was saying) is that upcoming changes shouldn't be comparing development to master, they should be comparing master to the last version.

@Samasaur1
Copy link
Owner Author

Samasaur1 commented Jun 3, 2022

Also, the other link comment was resolved when Danger finished

@Samasaur1
Copy link
Owner Author

I also need to update the required checks on PRs

@Samasaur1
Copy link
Owner Author

OK so I have seen the Danger inline comments/fails. I don't know why they don't work consistently (possibly because there were too many submitted at once/), but I think it's good enough. I'll also create an issue about making Danger work for other people's contributions.

@Samasaur1 Samasaur1 merged commit f8b229d into master Jun 4, 2022
@Samasaur1 Samasaur1 deleted the rework-ci-and-contributions branch June 4, 2022 15:29
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