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

Add sub actions #11

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

Add sub actions #11

wants to merge 7 commits into from

Conversation

simonsso
Copy link
Contributor

No description provided.

@simonsso simonsso requested a review from a team March 15, 2024 06:33
Copy link

@trsmarc trsmarc left a comment

Choose a reason for hiding this comment

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

The overall looks great and I understand the general direction, but I have a couple of questions about the file structure. Would you mind explaining the reasoning behind having three Dockerfiles and two entrypoint.sh files? I'm wondering if there might be a way to consolidate them for maintainability.

@simonsso
Copy link
Contributor Author

simonsso commented Mar 15, 2024

The overall looks great and I understand the general direction, but I have a couple of questions about the file structure. Would you mind explaining the reasoning behind having three Dockerfiles and two entrypoint.sh files? I'm wondering if there might be a way to consolidate them for maintainability.

I know there are copies, this is just quick and fast implementation to solve this. Feel free to fix it but there is no practical benefit of a clean up from a maintenance point of view.

One just edit all files with vim and run:

:argdo %s/v0.5.1/v0.6.1/|update

@trsmarc trsmarc requested a review from a team March 15, 2024 09:21
@ETeissonniere
Copy link
Member

@simonsso @trsmarc we can have one Dockerfile and entrypoint set to try-runtime, then each action can define its own args and pass them to the image.

Doing so will allow us to reuse the current prebuild step which will greatly improve CI time, and also reduce the number of docker images to pull when running CI - which also avoids CI taking too long.

Comment on lines +15 to +17
if [ -f "$runtime" ];then
try-runtime --runtime $runtime on-runtime-upgrade --checks=$checks $* snap --path $snap
fi
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure this will do what you want since if runtime == SKIP this will still evaluate to true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

check man [

then scroll down to;

       -f FILE
              FILE exists and is a regular file

Any non existing argument will skip

@@ -11,7 +11,7 @@ inputs:
description: 'Path to a previously taken snapshot, if does not exists we will create it'
default: '/tmp/default.snap'
runtime:
description: 'Path to a try-runtime enabled runtime to test migrations for'
description: 'Path to a try-runtime enabled runtime to test migrations for, Set to SKIP to skip test and only download snapshot'
required: true
Copy link
Member

Choose a reason for hiding this comment

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

You can change this to false if you want to make this option optional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a complication in how it is transferred to the Dockers' entrypoint with $1 $2 $3 $4

@simonsso
Copy link
Contributor Author

simonsso commented Mar 17, 2024

@simonsso @trsmarc we can have one Dockerfile and entrypoint set to try-runtime, then each action can define its own args and pass them to the image.

Doing so will allow us to reuse the current prebuild step which will greatly improve CI time, and also reduce the number of docker images to pull when running CI - which also avoids CI taking too long.

Maybe, I would just get his to work before we spend time to optimize it. I intend to put the actions in different CI jobs and foresee other bottle necks to have larger impact first.

The docker container is recreated in a few seconds in a job which will run for 10-20 minutes once every day or so. Not worth optimizing.

@simonsso simonsso enabled auto-merge (squash) March 18, 2024 00:21
@simonsso
Copy link
Contributor Author

simonsso commented Mar 19, 2024

The overall looks great and I understand the general direction, but I have a couple of questions about the file structure. Would you mind explaining the reasoning behind having three Dockerfiles and two entrypoint.sh files? I'm wondering if there might be a way to consolidate them for maintainability.

I created a ticket to look into this on a rainy day
CHA-454

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.

None yet

3 participants