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

Added --default-image option to be used with --shell-executor-no-image=false #1397

Merged
merged 3 commits into from
Nov 14, 2024

Conversation

NGPetrov
Copy link
Contributor

@NGPetrov NGPetrov commented Oct 24, 2024

Currently if --shell-executor-no-image=false is used the runner defaults to docker.io/ruby:3.1.
In case isolation is required, there is no option to run the ci in a custom image.

Added --default-image option.
If set when --shell-executor-no-image=false, the value will be used as a image source for the container.
If not set when --shell-executor-no-image=false, the default docker.io/ruby:3.1 will be used.
If set when --shell-executor-no-image=true (or not set), warring message is displayed.
If set when --shell-isolation, warring message is displayed.

Fixed typo (or old option name) in arvg.ts:49, from --no-shell-executor-no-image to --shell-executor-no-image

Included 3 tests to validate proper runner image/shell is used.

src/argv.ts Outdated Show resolved Hide resolved
src/argv.ts Outdated Show resolved Hide resolved
@NGPetrov NGPetrov force-pushed the feat/add-default-image-option branch from 7c6fa09 to d8db364 Compare October 27, 2024 09:18
src/job.ts Outdated Show resolved Hide resolved
src/argv.ts Outdated Show resolved Hide resolved
@NGPetrov NGPetrov force-pushed the feat/add-default-image-option branch from d8db364 to 2e8b659 Compare November 4, 2024 13:07
@NGPetrov
Copy link
Contributor Author

NGPetrov commented Nov 4, 2024

All fixes are implemented.
With a force push to keep the history clean.

@NGPetrov NGPetrov force-pushed the feat/add-default-image-option branch 5 times, most recently from f4b42e5 to 7fb0a2c Compare November 7, 2024 18:13
package.json Outdated
Copy link
Owner

Choose a reason for hiding this comment

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

What are these weird changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first version used had the parameter resolve to sting | null and the later check for argument clash was working fine.
After modifying the argument signature to get defaultImage() : string {, the argument clash check failed, as it treated the defaultImage as being always set.
I didn't thought it would be ok to remove the argument clash checks.
So I added another get to know, if the user explicitly set the default image, and so to check for incompatible arguments.

The need to modify it somehow was due to couple of tests failing.
The multiple force push-es due to my mistakes to push everything at once properly.

Copy link
Owner

@firecow firecow Nov 11, 2024

Choose a reason for hiding this comment

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

I'm talking about the specific no-op changes to package.json and package-lock.json 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"...due to my mistakes to push everything at once properly"
Sorry about that, a bit of distractions and I completely pushed (multiple times) the wrong changes.

Now all of it is removed.
There are no unnecessary changes to package*.json or the files produces from tests.

@NGPetrov NGPetrov force-pushed the feat/add-default-image-option branch from 7fb0a2c to b3cf0fe Compare November 11, 2024 11:24
package-lock.json Outdated Show resolved Hide resolved
@firecow firecow merged commit 295d7fe into firecow:master Nov 14, 2024
9 checks passed
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