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

Cleanup workflow templates; require 'with' key in module's .sync.yml #893

Merged
merged 3 commits into from
May 8, 2024

Conversation

h-haaks
Copy link
Contributor

@h-haaks h-haaks commented May 5, 2024

This is the final stage of removing the tight coupling between reusable gha workflow inputs and the templates in modulesync_config.

Fixes #887

@@ -2,8 +2,6 @@
# yamllint disable rule:line-length
.github/workflows/ci.yml:
excludes: []
pidfile_workaround: false
additional_packages: ''
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Figured these to be unnecessary as it's the default value defined in the reusable workflows.
The next msync run will remove these inputs from ci.yml.

<%- end -%>
<%- end -%>
<%- if @configs['with'].nil? || !@configs['with'].has_key?('allowed_owner') -%>
allowed_owner: '<%= @configs[:namespace] %>'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Figure this is whats blocking forks from releasing.
Added a test to prevent adding it double if declared in a modules .sync.yml

@h-haaks
Copy link
Contributor Author

h-haaks commented May 5, 2024

I have used github searches to find all voxpupuli modules using workflow inputs and modified .sync.yml to contain the required 'with' key.
See linked PRs in #887

@h-haaks
Copy link
Contributor Author

h-haaks commented May 5, 2024

Going trough the noop run I see that I have missed a few modules with my searches ...

I'll also have to look into how I write non string values in the templates. i.e. false changes to 'false'

@h-haaks h-haaks marked this pull request as draft May 5, 2024 00:57
@h-haaks
Copy link
Contributor Author

h-haaks commented May 5, 2024

Seems to be ready for review now. I'll fixup my commits before merge.

@h-haaks h-haaks marked this pull request as ready for review May 5, 2024 21:32
@binford2k
Copy link
Member

Hallo @h-haaks! Fill out this form if you'd like to claim your May the Source Be With You sticker! https://forms.office.com/r/Cn55uJmWMH

Copy link
Member

Choose a reason for hiding this comment

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

I think it can be reduced even further. Sending a diff is tricky, so here's the code:

jobs:
  puppet:
    name: Puppet
<%- if @configs['acceptance_tests'] && Dir[File.join(@metadata[:workdir], 'spec', 'acceptance', '**', '*_spec.rb')].any? -%>
    uses: voxpupuli/gha-puppet/.github/workflows/beaker.yml@v2
<%- else -%>
    uses: voxpupuli/gha-puppet/.github/workflows/basic.yml@v2
<%- end -%>
<%- if @configs['with'] -%>
    with:
<%- @configs['with'].each do |k,v| -%>
<%- if v.is_a?(String) -%>
      <%= k %>: '<%= v %>'
<%- else -%>
      <%= k %>: <%= v %>
<%- end -%>
<%- end -%>
<%- end -%>

You can even use v.inspect to avoid the if/else:

<%- @configs['with'].each do |k,v| -%>
      <%= k %>: <%= v.inspect %>
<%- end -%>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

inspect returns a double quoted string. That will generate lots of diffs/changes in the next msync run i guess.

irb(main):001:0> t = 'test'
=> "test"
irb(main):002:0> t.inspect
=> "\"test\""

Copy link
Member

Choose a reason for hiding this comment

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

Oh yes, I may have confused it with Python where repr(my_string) is actually using single quotes.

<%- end -%>
<%- end -%>
<%- end -%>
<%- if @configs['with'].nil? || !@configs['with'].has_key?('allowed_owner') -%>
Copy link
Member

Choose a reason for hiding this comment

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

This code is indeed intended to prevent running on forks. Isn't this equivalent?

Suggested change
<%- if @configs['with'].nil? || !@configs['with'].has_key?('allowed_owner') -%>
<%- unless @configs['with']&.has_key?('allowed_owner') -%>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, advanced ruby ...
I'll give it a try :)

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

It felt tempting to use to_yaml, but I think this is simple enough for now.

@h-haaks h-haaks merged commit cdcfe29 into voxpupuli:master May 8, 2024
4 checks passed
@h-haaks h-haaks deleted the cleanup-ci.yml.erb branch May 8, 2024 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove tight coupling to gha-puppet in gha workflow templates
3 participants