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

Show source image URL in summary #853

Conversation

tkdchen
Copy link
Contributor

@tkdchen tkdchen commented Mar 4, 2024

STONEBLD-1608

This is a rework for the reverted version. In this implementation, the source-build task writes the build result to a file inside workspace so that any other task can read it. Task summary reads source image URL from this file and if the result file does not exist yet, just skip silently.

The workspace mount is optional for task summary. It works for users who update task summary bundle but not PipelineRun YAML.

@tkdchen
Copy link
Contributor Author

tkdchen commented Mar 4, 2024

Tested with the following cases:

Case 1

  • source build is enabled

Result: task summary runs and source image URL is shown in summary.

Case 2

  • source build is not enabled

Result: task summary runs and source image URL is not shown in the summary logs.

Case 3

  • source build is enabled
  • pipeline is not updated by specifying workspace to summary task

Result: task summary runs and no source image URL is shown in the summary logs.

Comment on lines 54 to 57
if [ -e "$SOURCE_BUILD_RESULT_FILE" ]; then
found=
fields=($(cat "$SOURCE_BUILD_RESULT_FILE"))
for f in ${fields[@]}; do
if [ -n "$found" ]; then
echo "Generated Source Image is in : $(echo $f | tr -d '",')"
break
fi
if [ "$f" == '"image_url":' ]; then
found=yes
fi
done
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of parsing JSON in Bash, can the source-build task write something more easily parse-able to the workspace file? Maybe just the image url?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was intended to make the result file be general for sharing with other tasks, not only for summary even though only summary uses for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

make the result file be general for sharing with other tasks

IMO that's generally a bad idea. Files in the workspace can't really be trusted, since the user can always add a custom task that modifies the workspace content. That's also why the EC team is doing the Trusted Artifacts work #791

Copy link
Contributor

Choose a reason for hiding this comment

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

(although for the show-summary task this is fine, it's purpose is just to inform the user)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tkdchen tkdchen force-pushed the STONEBLD-1608-show-source-image-in-summary-rework branch from 8c9ee14 to e6333dc Compare March 4, 2024 10:00
@tkdchen tkdchen requested a review from chmeliik March 4, 2024 10:13
@tkdchen
Copy link
Contributor Author

tkdchen commented Mar 4, 2024

/test build-definitions-pull-request

1 similar comment
@tkdchen
Copy link
Contributor Author

tkdchen commented Mar 4, 2024

/test build-definitions-pull-request

@tkdchen tkdchen closed this Mar 5, 2024
@tkdchen tkdchen reopened this Mar 11, 2024
@tkdchen tkdchen force-pushed the STONEBLD-1608-show-source-image-in-summary-rework branch 2 times, most recently from ebb42b8 to e32ec50 Compare March 11, 2024 03:01
@tkdchen
Copy link
Contributor Author

tkdchen commented Mar 11, 2024

/retest

@tkdchen tkdchen force-pushed the STONEBLD-1608-show-source-image-in-summary-rework branch 2 times, most recently from f77363f to d4df87f Compare March 11, 2024 03:51
@tkdchen
Copy link
Contributor Author

tkdchen commented Mar 12, 2024

/retest

tkdchen added 2 commits March 13, 2024 12:56
STONEBLD-1608

This is a rework for the reverted version. In this implementation, the
source-build task writes the build result to a file inside workspace so
that any other task can read it. Task summary reads source image URL
from this file and if the result file does not exist yet, just skip
silently.

The workspace mount is optional for task summary. It works for users who
update task summary bundle but not PipelineRun YAML.

Signed-off-by: Chenxiong Qi <[email protected]>
@tkdchen tkdchen force-pushed the STONEBLD-1608-show-source-image-in-summary-rework branch from a3c3080 to 671dddc Compare March 13, 2024 04:56
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@tkdchen
Copy link
Contributor Author

tkdchen commented Mar 13, 2024

Rebased. No update to the changes.

@tkdchen
Copy link
Contributor Author

tkdchen commented Mar 13, 2024

/retest

@tkdchen tkdchen merged commit 2b39b1a into konflux-ci:main Mar 13, 2024
6 checks passed
@tkdchen tkdchen deleted the STONEBLD-1608-show-source-image-in-summary-rework branch March 13, 2024 07:18
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