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

Improve the manual "execute" customization #188

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

upils
Copy link
Collaborator

@upils upils commented Feb 22, 2024

Support args and env variables in the the "execute" customization to make it more useful.

See https://bugs.launchpad.net/ubuntu-image/+bug/2035216

@upils upils self-assigned this Feb 23, 2024
@upils upils force-pushed the improve-execute-customization branch from 71bba8e to 8aaa356 Compare March 18, 2024 16:38
Copy link

codecov bot commented Mar 18, 2024

Codecov Report

Attention: Patch coverage is 98.30508% with 1 line in your changes missing coverage. Please review.

Project coverage is 84.76%. Comparing base (43771d1) to head (64e04cd).

Files Patch % Lines
internal/statemachine/helper.go 90.90% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (43771d1) and HEAD (64e04cd). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (43771d1) HEAD (64e04cd)
unittests 2 1
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #188      +/-   ##
==========================================
- Coverage   93.99%   84.76%   -9.23%     
==========================================
  Files          18       18              
  Lines        3412     3466      +54     
==========================================
- Hits         3207     2938     -269     
- Misses        132      448     +316     
- Partials       73       80       +7     
Flag Coverage Δ
unittests 84.76% <98.30%> (-9.23%) ⬇️

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

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

@upils upils force-pushed the improve-execute-customization branch from a68d3d0 to 64561c2 Compare March 19, 2024 08:39
@upils upils marked this pull request as ready for review March 19, 2024 08:39
@upils upils requested a review from sil2100 March 19, 2024 08:40
Copy link
Collaborator

@sil2100 sil2100 left a comment

Choose a reason for hiding this comment

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

For now leaving a comment since I still need to think a bit more about this myself.

First of all: I see the addition of a test case for add-group here - is that related to the execute customization improvements? Feels a bit unfitting!

As for the change itself: I still need to think about this, but passing arguments as a list one-by-one feels a bit hm, artificial. It's certainly an option, but I'm thinking that maybe instead we should add a command: stanza or something where the user could simply execute the whole command to be run? Or maybe run:? While keeping path: there and slowly deprecating it (making it only run the command with no arguments). But I'm undecided myself yet. I'm thinking about this this way as imagine running a command that has 6 arguments to be passed - this would not really look nice or readable.

+1 on env though.

@upils
Copy link
Collaborator Author

upils commented Mar 21, 2024

First of all: I see the addition of a test case for add-group here - is that related to the execute customization improvements? Feels a bit unfitting!

I spotted the add-group customization was not tested and I added it. It was a bit careless and I will isolate it in a commit.

As for the change itself: I still need to think about this, but passing arguments as a list one-by-one feels a bit hm, artificial. It's certainly an option, but I'm thinking that maybe instead we should add a command: stanza or something where the user could simply execute the whole command to be run? Or maybe run:? While keeping path: there and slowly deprecating it (making it only run the command with no arguments). But I'm undecided myself yet. I'm thinking about this this way as imagine running a command that has 6 arguments to be passed - this would not really look nice or readable.

While doing it I was convinced that was a common way, but now that you mention it I checked several projects doing this kind of thing (ansible, terraform) and indeed the favored way is to gather everything under a single "command" field. Plus it looks indeed way easier to use.

As you mention we could add a "command" field (because it describes better what is expected) and we could redefine path as an alias to this field. So we will have a single underlying implementation and will be able let user migrate.

@upils upils force-pushed the improve-execute-customization branch from 64561c2 to b309a3a Compare April 3, 2024 12:46
@upils upils force-pushed the improve-execute-customization branch from 43e1b7c to f2b05a7 Compare April 11, 2024 10:26
@upils upils force-pushed the improve-execute-customization branch from f2b05a7 to ef371fd Compare May 2, 2024 10:12
sil2100
sil2100 previously approved these changes Jul 11, 2024
Copy link
Collaborator

@sil2100 sil2100 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 to me now, thank you for the adjustments!

@upils upils force-pushed the improve-execute-customization branch from ef371fd to ff6dc5c Compare July 16, 2024 12:09
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.

2 participants