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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: migrate Azure Pipelines to GitHub Actions #523

Merged
merged 10 commits into from Jun 3, 2021
Merged

ci: migrate Azure Pipelines to GitHub Actions #523

merged 10 commits into from Jun 3, 2021

Conversation

h1dden-da3m0n
Copy link
Contributor

@h1dden-da3m0n h1dden-da3m0n commented May 14, 2021

Description

As we discussed in Matrix the other day, here my proposal for a fully migrated CI from Azure Pipelines to GitHub Actions.
There are some requirements and discussions still required before we merge this, but this is a start.

Changes

  • migrate Azure Pipelines to GitHub Actions
  • add Release Drafter for easier GitHub Release management
  • update editorconfig slightly
  • add dependabot for pip and github-actions (let me know if the intervals seem alright to you)
  • update generate_xml.py to not rely on sub-path (i.e. second checkout)

Requirements

  • Add deployment secrets to Org and auth this repo to use them
    • DEPLOY_HOST - the host address of the deployment server
    • DEPLOY_USER - the user for the deployment to the server
    • DEPLOY_KEY - the ssh key of the user to auth against the server
  • Note: I assumed the deployment server to be the same as the one used for the Android client, hence why I used the same secret names

Notes

  • as an alterntive to the cobertura-action we could use codecov.io like we do in some other repos or just stick to what SonarCloud already prints out
  • check this test PR, if you need an example of what the checks and cobertura-action look like on a PR
  • Regarding release drafter and the release.yaml found within the repo (that too includes a changelog), I could add a basic manual workfow that 1. bumps the version number in that yaml and 2. updates the chancgelog based on the release draft created by Release Drafter. This would either auto commit to master or open a release-prep PR just before cutting a release. (Let me know what you thing/prefere 馃槈 )

Copy link
Member

@mcarlton00 mcarlton00 left a comment

Choose a reason for hiding this comment

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

Misc comments:

  • deployment server - afaik it's the same place. We can verify with Josh but I'm almost positive it is.
  • cobertura/codecov/sonarcloud - I don't have a whole lot of preference here. Looking at the output from cobertura just makes me feel bad, which I suppose might not be a bad things. But I don't think it should throw warnings about files that weren't touched in that PR. @oddstr13 might have more thoughts on these, he's worked more on testing than I have.
  • release.yaml - I'm not sure what you mean here. Right now when we're doing a release, we update release.yaml in a PR and then push a tag to kick off the release. If the file were updated automatically, I'm not sure how that could work. Though there's a good chance this is just me misunderstanding the release drafter bits

.github/workflows/build-publish.yaml Outdated Show resolved Hide resolved
.github/dependabot.yaml Show resolved Hide resolved
Comment on lines 58 to 59
- name: Create release Zip
run: zip -rq plugin.video.jellyfin-${{ matrix.py_version }}.zip plugin.video.jellyfin-${{ matrix.py_version }}
Copy link
Member

Choose a reason for hiding this comment

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

This is actually something I've been thinking about changing for a while. I may have mentioned in in chat, but I don't remember. In JellyCon I made a build script that combines the zipping with the addon.xml generation into one. If we're already changing the build system around, it might make sense to port that over here now. I'm open to suggestions about it.

Refs:

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 will have a look!

(btw. woudn't #405 be a ref too?)

Copy link
Member

Choose a reason for hiding this comment

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

It would, yes. It stalled out because we couldn't agree on the dependencies, and then I ended up writing my own for JellyCon that combined our approaches and #405 never got picked back up afterwards.

@h1dden-da3m0n
Copy link
Contributor Author

h1dden-da3m0n commented May 15, 2021

Regarding the release.yaml, given your description of the release process I could add a manually trigger-able workflow that opens the 'release bump' PR with a updated release.yaml that already has the new version and changeglog setup. (e.g. PR https://github.com/h1dden-da3m0n/jellyfin-plugin-tmdb/pull/8, an example from me toying around for the Plugin CI update).

I hope that is a bit clearer, but feel free to ping me here on Matrix regarding the idea if you still have questions.

* Note: I would not setup a comment based merging though (as my example suggests) that was just from me experimenting

@mcarlton00
Copy link
Member

Bot PRs updating release.yaml would be pretty great if that can be worked out. It'd be nice to be able to push a release without needing multiple members online. In that situation, what would trigger the bot to make the PR?

@h1dden-da3m0n
Copy link
Contributor Author

h1dden-da3m0n commented May 15, 2021

Bot PRs updating release.yaml would be pretty great if that can be worked out. It'd be nice to be able to push a release without needing multiple members online. In that situation, what would trigger the bot to make the PR?

In theory, anyone with the right permissions can trigger the workflow to create the PR. That PR then would just need to be merged and could trigger whatever is needed to actually trigger a release (i.e. create and push a tag with the right version?)

By that all you need is hit the review count and you could release

Copy link
Member

@oddstr13 oddstr13 left a comment

Choose a reason for hiding this comment

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

Just one thing; How about also publishing the artifact to the GitHub release? (Or does this already do that, and I missed how it's done?)

@oddstr13
Copy link
Member

For coverage, the diff is most important in PRs, and codecov calculates this
Examples from last time I was messing with this;
oddstr13#2 (comment)
oddstr13#3 (comment)

@h1dden-da3m0n
Copy link
Contributor Author

For coverage, the diff is most important in PRs, and codecov calculates this
Examples from last time I was messing with this;
oddstr13#2 (comment)
oddstr13#3 (comment)

That is what I thought and why I hoped we could use that one instead ;)
For Flake8 reviewdog should handle the diff already and send the appropriate Check result for PRs.

I will finish up the build.py so that it includes zipping the files, see what is to do for Codecov. I will ping one of you on Matrix once done (some when during this week 馃 )

@h1dden-da3m0n h1dden-da3m0n marked this pull request as draft May 23, 2021 18:42
@h1dden-da3m0n h1dden-da3m0n marked this pull request as ready for review May 28, 2021 22:57
@h1dden-da3m0n
Copy link
Contributor Author

Okay I have everything in, however 1 concern that I would like to discuss: The Create Release PR works fine and creates a PR to pump the release.yaml, however the On PR Merge workflow is a bit wonky and I am not 100% sure I like it or that its needed.

Cause all that one would need to do if you want to release a new is trigger the Create Release PR get it merged and create & push a tag on the merge commit from your local machine.

If you have any questions feel free to comment here or fire them my way on matrix.

@mcarlton00
Copy link
Member

After moving the build.py into .build it's now failing because it can't find the release.yaml.

% python .build/build.py
Traceback (most recent call last):
  File "/home/matt/Projects/jellyfin/kodi/jellyfin-kodi/.build/build.py", line 120, in <module>
    with open(config_path, 'r') as fh:
FileNotFoundError: [Errno 2] No such file or directory: '/home/matt/Projects/jellyfin/kodi/jellyfin-kodi/.build/release.yaml'

I think I prefer to have the build script in the root of the repo in general, so probably revert 326f009 instead of having to rewrite stuff to look in a parent directory.

Copy link
Member

@mcarlton00 mcarlton00 left a comment

Choose a reason for hiding this comment

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

Reviewing again, if I'm following all the actions correctly:

  • Create release PR - triggered manually and will update the version/changelog in release.yaml and create a PR for it
  • On PR merge - only applies if a PR has the release-prep label, but will automatically push a tag to the repo
  • Release drafter - Triggered by pushed tags, creates a draft release that we can then turn into a proper release

Correct?

.github/workflows/create-release-pr.yaml Outdated Show resolved Hide resolved
@h1dden-da3m0n
Copy link
Contributor Author

h1dden-da3m0n commented May 31, 2021

Reviewing again, if I'm following all the actions correctly:

* Create release PR - triggered manually and will update the version/changelog in `release.yaml` and create a PR for it

* On PR merge - only applies if a PR has the `release-prep` label, but will automatically push a tag to the repo

* Release drafter - Triggered by pushed tags, creates a draft release that we can then turn into a proper release

Correct?

In its current form, yes, besides the release drafter aspect.
Release Drafter will run on every push to the default branch and update and maintain a release draft based on the merged PRs since the past release. It will also categorize them if the PRs are labelled with labels configured in the Release Drafter config (if you need an example of that: jellyfin-plugin-tvmaze and somewhere outside the org where I use it rest-list-parameter-plugin)

However, I have thought about the workflow a bit. Based on the fact that my initial testing, especially with the release-prep was not consistent I have a alternative proposition for the release workflow:

  1. keep the 'prepare-release' PR workflow to increment the version in release.yaml
  2. replace the On PR Merge workflow with a dedicated and manually trigger able workflow to actually release the kodi plugin and publish the GH release. (I will have to check who can trigger the manual workflows, but I think everyone in the org may be able too 馃 )

If you are fine with this proposal then I shall update the respective workflows

@mcarlton00
Copy link
Member

I think a manual run to do a release/publish makes sense. While everything being automagical is nice, needing to click a button to make it more reliable is 100% a trade off I'm willing to make.

@h1dden-da3m0n
Copy link
Contributor Author

okay moved the build.py back to root, simplified the workflows by spiting them up and making them more reliable (the release now works as we discussed above)

if you have any further questions feel free and I shall answer as soon as I can.

@sonarcloud
Copy link

sonarcloud bot commented Jun 1, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@mcarlton00 mcarlton00 merged commit 399fc32 into jellyfin:master Jun 3, 2021
@h1dden-da3m0n h1dden-da3m0n deleted the ci/migrate-to-actions branch June 4, 2021 18:13
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.

None yet

3 participants