Rewrite GHA workflow using reusable actions

[archanaserver]

For reference: #191

[ekohl]

This is breaking things:

foreman_salt/Rakefile

Line 23 in 345cca0

APP_RAKEFILE = File.expand_path('test/dummy/Rakefile', __dir__)

@nadjaheitmann mind having a look?

[nadjaheitmann]

@nadjaheitmann mind having a look?

Looks like some artifact from the plugin template. I'll have a look.

[bastian-src]

This is breaking things:

foreman_salt/Rakefile

Line 23 in 345cca0

APP_RAKEFILE = File.expand_path('test/dummy/Rakefile', __dir__)

IMHO, this line can be removed. As @nadjaheitmann mentions, it came from the plugin template but the actual test/dummy/Rakefile has never been included in this repo.

In contrast, Foreman Ansible added the test/dummy/Rakefile in the initial commit: theforeman/foreman_ansible@fc7e244#diff-128d579f540e340778f9670e403a75f9d6ac1d2821f4d1adae3883d21864aac8R1

and removes it shortly after it, in a cleanup commit: theforeman/foreman_ansible@e2cc1b5#diff-128d579f540e340778f9670e403a75f9d6ac1d2821f4d1adae3883d21864aac8L1

Still, I don't understand why this issue didn't occur earlier.

[nadjaheitmann]

Thanks @bastian-src !!
@archanaserver Just try and remove the line in your PR. I don't see where it is used, either.

[archanaserver]

Thanks everyone! I have made the changes as suggested, also the below line was causing another issue and I find it didn't have any dependencies so removed that also, which was causing another failure, Lmk if I'm missing something here.

foreman_salt/Rakefile

Line 24 in 345cca0

load 'rails/tasks/engine.rake'

And to continue running the workflow, we need rubocop/rake_task which I have raised changes in another PR and to make this PR clean I have moved the Remove APP_RAKEFILE assignment changes here as well: #193

[ekohl]

It's ok to include another PR in here. A PR can have multiple commits. In this case I think it'd be more obvious

[nadjaheitmann]

It's ok to include another PR in here. A PR can have multiple commits. In this case I think it'd be more obvious

Sounds like a good idea. It would be nice to have a working pipeline for rubocop since this is what you are fixing here.

[ekohl]

I'd also expect that https://github.com/theforeman/foreman_salt/blob/master/.github/workflows/unit_tests.yml would be rewritten to use the reusable actions. You can probably merge both flows into a single file, like we do with other plugins.

[sbernhard]

Is this done completely? Or does it need another PR to work?

[ekohl]

I think this fails because it doesn't run rake webpack:compile. You can see it's guarded here:
https://github.com/theforeman/actions/blob/bda4cbd6e077caefc3bcffd68b481af49874674c/.github/workflows/foreman_plugin.yml#L139-L142

So this is a case where there are integration tests that use a browser but the plugin itself doesn't have webpack.

@evgeni can we also detect test/integration/* and run webpack:compile in our common action?

[evgeni]

I think this fails because it doesn't run rake webpack:compile. You can see it's guarded here: https://github.com/theforeman/actions/blob/bda4cbd6e077caefc3bcffd68b481af49874674c/.github/workflows/foreman_plugin.yml#L139-L142

So this is a case where there are integration tests that use a browser but the plugin itself doesn't have webpack.

@evgeni can we also detect test/integration/* and run webpack:compile in our common action?

We can. Or should we use app/assets? This will also install/compile webpack for plugins that do not have integration tests tho.

[ekohl]

I'd prefer test/integration since app/assets is for the classic asset pipeline. Note this plugin doesn't have a webpack directory so it's really just compiling the core Foreman webpack. There's no benefit in testing that here.

[evgeni]

Fair. Try theforeman/actions#33

[evgeni]

Seems that fixed it, nice.

[archanaserver]

I think this fails because it doesn't run rake webpack:compile. You can see it's guarded here: https://github.com/theforeman/actions/blob/bda4cbd6e077caefc3bcffd68b481af49874674c/.github/workflows/foreman_plugin.yml#L139-L142

So this is a case where there are integration tests that use a browser but the plugin itself doesn't have webpack.

@evgeni can we also detect test/integration/* and run webpack:compile in our common action?

@ekohl, I'm a bit unclear on the issue. Could you help me understand?

I want to make sure I understand the issue correctly and the fix you and @evgeni did here.

[evgeni]

@archanaserver this plugin only has "old style" (sprockets) based assets (there is no webpack folder and no package.json), so our shared action skipped installation and compilation of webpack based things. However, this test does have integration tests that require the whole app to work, and because Foreman itself does have webpack, we had to adjust the action to install NodeJS and compile webpack assets, even if they are not required for this plugin itself as otherwise the integration tests were not passing.

[archanaserver]

Make sense now, thanks @evgeni!

[evgeni]

with my "we should use the shared actions workflow for all plugins" hat, this looks good.

one small suggestion, but not a blocker.

@theforeman/salt please weigh in, I would prefer your ack over mine ;-)

[evgeni]

I would include SALT-* here:

Suggested change

	- master
	- 'master'
	- 'SALT-*'

but it's up to the real maintainers of this plugin to decide :)

[ekohl]

Sounds like a good idea, but also something that isn't here today so I wouldn't consider it blocking.

[sbernhard]

Thanks @archanaserver & @ekohl & @evgeni