-
Notifications
You must be signed in to change notification settings - Fork 35
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 GHA CI workflow #62
base: master
Are you sure you want to change the base?
Conversation
79f455a
to
975c327
Compare
I had this doubt earlier, how can we pass this error:
and without resolving this, how can we check if rubocop is passing or not here #60 #61? What am I missing here @ekohl? |
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.
and without resolving this, how can we check if rubocop is passing or not here #60 #61? What am I missing here @ekohl?
I would not run the workflow at all and rename it. So I've left a comment on the template file to rename. Then you modify rename.rb
to rename it when a user actually creates a plugin, similar to this:
foreman_plugin_template/rename.rb
Line 65 in ddd8127
FileUtils.mv('README.plugin.md', 'README.md') |
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 doesn't appear to replace the content in the file. Probably because line 35 in rename.rb
also matches .github
.
rename.rb
Outdated
@@ -63,6 +63,7 @@ def usage | |||
FileUtils.rm_rf(old_dirs) | |||
|
|||
FileUtils.mv('README.plugin.md', 'README.md') | |||
FileUtils.mv('ci.yml.tpl', 'ci.yml') |
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 fails for me:
$ ./rename.rb foreman_best_plugin
/usr/share/ruby/fileutils.rb:1181:in `rename': No such file or directory @ rb_file_s_rename - (ci.yml.tpl, ci.yml) (Errno::ENOENT)
from /usr/share/ruby/fileutils.rb:1181:in `block in mv'
from /usr/share/ruby/fileutils.rb:2481:in `block in fu_each_src_dest'
from /usr/share/ruby/fileutils.rb:2497:in `fu_each_src_dest0'
from /usr/share/ruby/fileutils.rb:2479:in `fu_each_src_dest'
from /usr/share/ruby/fileutils.rb:1172:in `mv'
from ./rename.rb:66:in `<main>'
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.
You should use Find.prune
if the directory is .git
. https://docs.ruby-lang.org/en/master/Find.html documents it.
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.
FileUtils.mv('ci.yml.tpl', 'ci.yml') | |
FileUtils.mv('.github/workflows/ci.yml.tpl', '.github/workflows/ci.yml') |
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.
And please also drop the Jenkins integration while you're at it. At least this:
foreman_plugin_template/lib/tasks/foreman_plugin_template_tasks.rake
Lines 45 to 48 in d9ca190
load 'tasks/jenkins.rake' | |
if Rake::Task.task_defined?(:'jenkins:unit') | |
Rake::Task['jenkins:unit'].enhance ['test:foreman_plugin_template', 'foreman_plugin_template:rubocop'] | |
end |
Edit: #59 has pretty much what's needed.
#62 (comment) is still relevant. Please follow the README and pretend to a user of it. You can even consider writing some CI for the repo itself where it calls |
rename.rb
Outdated
@@ -63,6 +63,7 @@ def usage | |||
FileUtils.rm_rf(old_dirs) | |||
|
|||
FileUtils.mv('README.plugin.md', 'README.md') | |||
FileUtils.mv('ci.yml.tpl', 'ci.yml') |
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.
FileUtils.mv('ci.yml.tpl', 'ci.yml') | |
FileUtils.mv('.github/workflows/ci.yml.tpl', '.github/workflows/ci.yml') |
.github/workflows/ci.yml
Outdated
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's most important in this repository that you can use rename.rb
, so I think we should also see that step in CI.
.github/workflows/test_rename.yml
Outdated
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'd have expected this in the regular CI workflow. If not, rename.rb
must remove it.
.github/workflows/test_rename.yml
Outdated
|
||
- name: Run rename.rb | ||
run: | | ||
chmod +x rename.rb |
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.
rename.rb
is already executable
chmod +x rename.rb |
.github/workflows/test_rename.yml
Outdated
|
||
steps: | ||
- name: Checkout Repository | ||
uses: actions/checkout@v2 |
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.
v2 is very old. I think v4 is the latest
uses: actions/checkout@v2 | |
uses: actions/checkout@v4 |
.github/workflows/ci.yml.tpl
Outdated
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.
Currently I don't see any difference compared to ci.yml
so now there would be no value in implementing it as a template. Depending on how you handle the test_rename.yml
comments it may or may not make sense to keep it.
.github/workflows/test_rename.yml
Outdated
if [ ! -f "README.md" ]; then | ||
echo "README.md not found!" | ||
exit 1 | ||
fi | ||
|
||
if [ ! -f ".github/workflows/ci.yml" ]; then | ||
echo ".github/workflows/ci.yml not found!" | ||
exit 1 | ||
fi |
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.
Both of these are also present without running rename so they're a poor test. You should really test for the content.
@ekohl seems like i don't understand what we talked earlier about writing CI for rename.rb script. can you guide me more? |
if File.basename(path) == '.git' | ||
Find.prune | ||
elsif File.file?(path) | ||
system(%(sed -i 's/foreman_plugin_template/#{snake}/g; s/ForemanPluginTemplate/#{camel}/g; s/foremanPluginTemplate/#{camel_lower}/g' #{path})) |
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 line should now fix this issue: #63
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'd like it if it was at least a separate commit for visibility.
The script is working fine now, @ekohl do you mind looking into it? |
exit 1 | ||
fi | ||
|
||
if ! grep -q "$PLUGIN_NAME" "$file"; then |
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 would also assert that foreman_plugin_template
, ForemanPluginTemplate
and foremanPluginTemplate
do not show up anymore.
|
||
rename_test: | ||
name: Rename Script Test | ||
needs: [rubocop, 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.
I think this should be independent. Often a reason to make it depend on others is to save on CI costs, but the rename is very cheap. In fact, I would probably turn it around: make the tests depend on renaming.
Find.find('.') do |path| | ||
# Change all the paths to the new snake_case name | ||
if path =~ /foreman_plugin_template/i | ||
new = path.gsub('foreman_plugin_template', snake) | ||
# Recursively copy the directory and store the original for deletion | ||
# Check for $ because we don't need to copy template/hosts for example |
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.
Why do you drop this comment?
|
||
env: | ||
PLUGIN_NAME: foreman_plugin_template | ||
FILES_TO_CHECK: "README.md,.github/workflows/ci.yml" |
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 would also check if a file rename worked. For example, you could check that find -name '*foreman_plugin_template*'
doesn't show any files and verify the new files do exist.
runs-on: ubuntu-latest | ||
|
||
env: | ||
PLUGIN_NAME: foreman_plugin_template |
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 is exactly the same name as the template, so a rename is not going to do anything.
No description provided.