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

Update the worker to make it more generic #67

Merged
merged 4 commits into from
Aug 9, 2023

Conversation

r0x0d
Copy link
Member

@r0x0d r0x0d commented Aug 7, 2023

The idea behind this commit is to make the worker even more generic, not only being able to execute bash scripts, but, let the user decide which type of script it will be executed, by defining a interpreter key in the partial yaml playbook, which will tell the worker what to execute.

Changes included in this commit:

  • Rename every reference from rhc-worker-bash to rhc-worker
  • Added the interpreter key to be parsed in the incoming playbook
    ** Use the interpreter key instead of hardcoding the /bin/bash path
    for the command execution
  • Updated the references in the project to build using the new name

The idea behind this commit is to make the worker even more generic, not
only being able to execute bash scripts, but, let the user decide which
type of script it will be executed, by defining a `interpreter` key in
the partial yaml playbook, which will tell the worker what to execute.

Changes included in this commit:

* Rename every reference from rhc-worker-bash to rhc-worker
* Added the `interpreter` key to be parsed in the incoming playbook
** Use the `interpreter` key instead of hardcoding the /bin/bash path
    for the command execution
* Updated the references in the project to build using the new name

Signed-off-by: Rodolfo Olivieri <[email protected]>
@r0x0d r0x0d requested a review from andywaltlova August 7, 2023 19:24
@r0x0d r0x0d self-assigned this Aug 7, 2023
@codecov-commenter
Copy link

Codecov Report

Patch coverage: 81.81% and no project coverage change.

Comparison is base (b59b767) 66.47% compared to head (856f214) 66.47%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #67   +/-   ##
=======================================
  Coverage   66.47%   66.47%           
=======================================
  Files           5        5           
  Lines         343      343           
=======================================
  Hits          228      228           
  Misses         97       97           
  Partials       18       18           
Flag Coverage Δ
go-1.16 66.47% <81.81%> (ø)
go-1.20 66.47% <81.81%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/main.go 0.00% <0.00%> (ø)
src/runner.go 81.39% <100.00%> (ø)
src/util.go 74.22% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@andywaltlova andywaltlova left a comment

Choose a reason for hiding this comment

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

I am also thinking if the rhc-worker name is okay from insights POV, are they okay with it? I would expect something like rhc-worker-multipurpose or rhc-worker-generic because of the existence of rhc-worker-playbook, I mean .. the specific name just implies better what is the purpose of the package I think.

Apart from the few questions, the changes looks good to me!

r0x0d and others added 2 commits August 8, 2023 09:04
Co-authored-by: Andrea Waltlová <[email protected]>
Signed-off-by: Rodolfo Olivieri <[email protected]>
@andywaltlova
Copy link
Collaborator

Alright rhc-worker-script sounds reasonable

Copy link
Collaborator

@andywaltlova andywaltlova left a comment

Choose a reason for hiding this comment

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

  • server.go docstring for Send method needs update (still mentions executing bash script)
  • runner.go L92 docstring as well
  • CONTRIBUTING file also mentions bash
  • Summary in specfile still mentions bash

README.md Outdated

- [RHC Worker Bash](#rhc-worker-bash)
- [RHC Worker Bash](#rhc-worker-script)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- [RHC Worker Bash](#rhc-worker-script)
- [RHC Worker Script](#rhc-worker-script)

@andywaltlova
Copy link
Collaborator

No builds for https://copr.fedorainfracloud.org/coprs/g/oamg/rhc-worker-script/builds/ yet, I don't know enough about github actions to know if it will be triggered in this PR or only after we merge it - I don't think it should block the merge :)

@packit-as-a-service
Copy link

We have requested the builder permissions for the @oamg/rhc-worker-script Copr project.

Please confirm the request on the @oamg/rhc-worker-script Copr project permissions page and retrigger the build by a /packit build pull-request comment or click on a Re-run button.

2 similar comments
@packit-as-a-service
Copy link

We have requested the builder permissions for the @oamg/rhc-worker-script Copr project.

Please confirm the request on the @oamg/rhc-worker-script Copr project permissions page and retrigger the build by a /packit build pull-request comment or click on a Re-run button.

@packit-as-a-service
Copy link

We have requested the builder permissions for the @oamg/rhc-worker-script Copr project.

Please confirm the request on the @oamg/rhc-worker-script Copr project permissions page and retrigger the build by a /packit build pull-request comment or click on a Re-run button.

@r0x0d
Copy link
Member Author

r0x0d commented Aug 9, 2023

/packit build

Copy link
Collaborator

@andywaltlova andywaltlova left a comment

Choose a reason for hiding this comment

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

Looks good now!

@r0x0d r0x0d merged commit 00082b3 into oamg:main Aug 9, 2023
@r0x0d r0x0d deleted the make-it-more-generic branch August 9, 2023 12:32
@r0x0d r0x0d mentioned this pull request Aug 10, 2023
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.

3 participants