-
Notifications
You must be signed in to change notification settings - Fork 90
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
Flatten stacks with parent in non-terminating-image checks #574
base: main
Are you sure you want to change the base?
Flatten stacks with parent in non-terminating-image checks #574
Conversation
Signed-off-by: thepetk <[email protected]>
Signed-off-by: thepetk <[email protected]>
Signed-off-by: thepetk <[email protected]>
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: thepetk The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple comments/questions!
@@ -81,7 +81,9 @@ jobs: | |||
run: echo "TEST_DELTA=true" >> $GITHUB_ENV | |||
|
|||
- name: Check that containers components are non terminating | |||
run: bash tests/check_non_terminating.sh | |||
run: | | |||
go build -C tests/check_non_terminating |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with the -C
flag and I can't find much on it, what does it do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to avoid changing dirs with cd so I used the flag -C to change it on build -> here
tests/check_non_terminating.sh
Outdated
@@ -7,6 +7,9 @@ DEVFILES_DIR="$(pwd)/stacks" | |||
# The stacks to test as a string separated by spaces | |||
STACKS=$(bash "$(pwd)/tests/get_stacks.sh") | |||
|
|||
# Path to the check_non_terminating go package | |||
NON_TERMINATING_MODULE_BIN="$(pwd)/tests/check_non_terminating/./check_non_terminating" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NON_TERMINATING_MODULE_BIN="$(pwd)/tests/check_non_terminating/./check_non_terminating" | |
NON_TERMINATING_MODULE_BIN="$(pwd)/tests/check_non_terminating/check_non_terminating" |
Do we need the .
there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also if the second check_non_terminating
is the binary name maybe we rename it for clarity? So it isn't doubled up in the path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made it a bit more dynamic and gave it the name flatten-parent
as default (also for the workflow).
Signed-off-by: thepetk <[email protected]>
Description of Changes
The PR fixes the issue that the
check_non_terminating
script faces for stacks that include a parent. An example is #401 where the changes introduced are failing.The list of updates:
Related Issue(s)
fixes devfile/api#1568
Acceptance Criteria
Have you read the devfile registry contributing guide and followed its instructions?
Does this repository's tests pass with your changes?
Does any documentation need to be updated with your changes?
Have you tested the changes with existing tools, i.e. Odo, Che, Console? (See devfile registry contributing guide on how to test changes)
Tests Performed
Explain what tests you personally ran to ensure the changes are functioning as expected.
How To Test
Notes To Reviewer
I've tried to mimic the updates introduced by the PR for ollama and ran the workflow here: https://github.com/thepetk/registry/actions/runs/13076640505/job/36490341149