-
Notifications
You must be signed in to change notification settings - Fork 71
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
Cleanup workflow templates; require 'with' key in module's .sync.yml #893
Conversation
@@ -2,8 +2,6 @@ | |||
# yamllint disable rule:line-length | |||
.github/workflows/ci.yml: | |||
excludes: [] | |||
pidfile_workaround: false | |||
additional_packages: '' |
There was a problem hiding this comment.
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] %>' |
There was a problem hiding this comment.
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
I have used github searches to find all voxpupuli modules using workflow inputs and modified .sync.yml to contain the required 'with' key. |
f93a1d9
to
c5f3e04
Compare
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' |
Seems to be ready for review now. I'll fixup my commits before merge. |
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 |
There was a problem hiding this comment.
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 -%>
There was a problem hiding this comment.
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\""
There was a problem hiding this comment.
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') -%> |
There was a problem hiding this comment.
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?
<%- if @configs['with'].nil? || !@configs['with'].has_key?('allowed_owner') -%> | |
<%- unless @configs['with']&.has_key?('allowed_owner') -%> |
There was a problem hiding this comment.
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 :)
There was a problem hiding this 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.
This is the final stage of removing the tight coupling between reusable gha workflow inputs and the templates in modulesync_config.
Fixes #887