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

Add github workflow to run PDK tests #176

Merged
merged 5 commits into from
Apr 11, 2022
Merged

Conversation

nward
Copy link
Collaborator

@nward nward commented Apr 9, 2022

Create a GitHub workflow which runs PDK tests - just using the workflow example from https://github.com/mysociety/action-pdk for now.
This workflow currently fails because of the issue that will be fixed in #174.

Github actions is good - there are other options available, but this is super low effort to make go. If we find it lacking we can look at others - but I think if we look at using GH Actions for doing releases then it's reasonable to use it for testing, too.

@djjudas21
Copy link
Owner

GitHub actions would be my preferred option - I'm using it in a few places for testing and deployment of other projects (python and kubernetes based). I'll pick this up on Monday as I'm away for the weekend 🙂

@nward
Copy link
Collaborator Author

nward commented Apr 9, 2022

Great stuff! There's a few PRs from me today (and at least 1 more to come). None are urgent, of course. Enjoy your weekend away!

@djjudas21
Copy link
Owner

Hope you don't mind, I've tweaked this a little. I've switched to a different provider of the PDK actions which seemed a bit better wrapped and allows for testing across multiple Puppet versions etc.

There doesn't seem to be a performance benefit to using these other actions, but I set the validate and test actions to run in parallel rather than sequentially, and it knocks about a minute off the total pipeline.

@djjudas21
Copy link
Owner

I've also added an action for automatically publishing to Puppet Forge when a new tag is created which matches a semver notation with a v prefix. To date, I haven't been using the v prefix in tags/releases. I guess this part of the pipeline will only get tested in anger when we come to make our next tagged release.

@djjudas21 djjudas21 self-requested a review April 10, 2022 20:37
djjudas21
djjudas21 previously approved these changes Apr 10, 2022
Copy link
Owner

@djjudas21 djjudas21 left a comment

Choose a reason for hiding this comment

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

I'm happy to approve this as it seems to work properly, but I'll let you merge it so you can read/comment on any part before it's finalised.

@nward
Copy link
Collaborator Author

nward commented Apr 10, 2022

This all looks good to me. I've updated my original branch here: https://github.com/SearchLightNZ/puppet-freeradius/tree/add_test_github_test_workflow

No functional changes - but I've:

  • Removed the merge main commit, and rebased the branch on main
  • Dropped my original commit and squashed your two creating the test/validate workflows

This is probably a Git workflow/style debate.
On rebasing:
I like to rebase rather than merge, as the git history graph can get really messy. I work with a bunch of people who are not git-minded, and found this workflow makes the history a lot more accessible for them.
I also like to rebase right before a PR merge (merge button has a "Rebase and Merge" option), for the same reason - branches going on in parallel makes for a complicated history.

On squashing:
I like to squash iterative commits. Again for clean history reasons - it makes it easier to walk back through. I think if we have a situation like this where we are testing one option or another, we can mention that in the final commit (and of course in this discussion in the PR).

What do you think? (About my git philosophical ramblings, and the tweaked branch :)

@nward
Copy link
Collaborator Author

nward commented Apr 11, 2022

Doing some testing of this with my branch for #162 which has tests for Debian osfamily as well, and this doesn't seem to be working that well. I can run it all manually and it goes fine, but for some reason running it through GH actions doesn't play nice.. will report more when I have it..

@djjudas21
Copy link
Owner

Looks good to me. I've always leaned towards merging rather than rebasing, although I think this is just because I learned about merging before rebasing, rather than for any philosophical reason. Perfectly happy with rebasing here, and I also like clean history and atomic commits (once it reaches main, anyway).

My instinct is to merge this now it's working, rebase or merge your branch for #162, and then fix that branch over there (whether it be the tests or the code).

@nward
Copy link
Collaborator Author

nward commented Apr 11, 2022

Yep, that sounds reasonable. Turns out the issue isn't unique to this repo either: https://github.com/nward/puppet_pdk_actions_bug

Re. rebasing - there's also a bit of a theory that rebasing is scary and should never be done etc. - which is generally true if you're working on a branch with other people, but if you're working alone or can coordinate well then that's no longer true. Most modern git workflows recommend rebasing like this. It's different if you use git to manage infrastructure or something and need it as an audit trail, of course, but that's not what's happening here !

@nward nward force-pushed the add_test_github_test_workflow branch from b561c9d to e092b32 Compare April 11, 2022 08:16
@nward
Copy link
Collaborator Author

nward commented Apr 11, 2022

OK - I was able to force push my changes and if it all runs fine it will need an approval then we can merge.

Incidentally, I thought I had created this PR from SearchLightNZ/puppet-freeradius - did the source branch change when you committed changes, or did I accidentally create this as a branch on djjudas21/puppet-freeradius?

@djjudas21 djjudas21 self-requested a review April 11, 2022 11:28
@djjudas21
Copy link
Owner

This branch was created on djjudas21/puppet-freeradius (which I'm happy for you to do)

@nward
Copy link
Collaborator Author

nward commented Apr 11, 2022

Ah! I have been intending to do them on my one. I'll keep trying to do that, but, glad it won't annoy you if I typo it.. :-)

@nward nward merged commit e5e9a29 into main Apr 11, 2022
@nward nward deleted the add_test_github_test_workflow branch April 11, 2022 11:34
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