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

feat(publishReports): add failIfEmpty parameter #853

Conversation

lemeurherve
Copy link
Member

This PR adds an optional failIfEmpty parameter to fail the call to publishReports if the file to upload is empty. (To avoid erasing an existing report for example).
The value of this parameter is set to false by default to keep the current behavior for existing consumers not needing it.

This PR also regroups the two sh blocks into one, and adds braces & double quotes around variables to prevent globbing and words splitting.

Ref:

@lemeurherve lemeurherve requested a review from a team May 2, 2024 19:17
@lemeurherve lemeurherve marked this pull request as draft May 2, 2024 19:25
@dduportal
Copy link
Contributor

This PR also regroups the two sh blocks into one

Can you revert this change please? having this particular operations (az <...>) separated allowed to check the time spent on each one easily and gives better.

lineCount=$(wc -l "${filename}" | awk '{print $1}')
if [[ "${failIsEmpty}" != false && "${lineCount}" -eq 0 ]]; then
echo "ERROR: ${filename} is empty."
exit 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Fundamental question: do we want the calling pipeline to fail? At first sight it looks like but since the only known use case today is jenkins-infra/helpdesk#4056 then I'm asking.

Is the condition (and its maintenance burden: it is not even tested since it is implemented with shell code) worth the gain considering PHS reports are now always generated?

Copy link
Member Author

Choose a reason for hiding this comment

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

So... rename this PR and change failIfEmpty by skipIfEmpty?

@lemeurherve
Copy link
Member Author

This PR also regroups the two sh blocks into one

Can you revert this change please? having this particular operations (az <...>) separated allowed to check the time spent on each one easily and gives better.

I propose to remove the az storage blob call instead, cf:

@dduportal
Copy link
Contributor

Closing as per the proposal in #853 (comment)

@dduportal dduportal closed this May 27, 2024
@lemeurherve
Copy link
Member Author

Closing as per the proposal in #853 (comment)

FWIW this proposal was a response to #853 (comment), not a proposal to close this PR.

@dduportal
Copy link
Contributor

Closing as per the proposal in #853 (comment)

FWIW this proposal was a response to #853 (comment), not a proposal to close this PR.

Ack. it was not clear to me. I'm reopening this PR but it has to be worked on or closed for no activity: having a draft PR with no work opened for weeks shows maintainers cannot afford working on a given subject. Closing such PRs shows we acknowledge this and we can focus our effort on something else.

@dduportal dduportal reopened this May 27, 2024
@lemeurherve
Copy link
Member Author

lemeurherve commented May 27, 2024

having a draft PR with no work opened for weeks shows maintainers cannot afford working on a given subject

I agree, but last two weeks I was in PTO so really there has been only 4 working days elapsed since our last exchange on this PR, which is not a lot.

@dduportal
Copy link
Contributor

having a draft PR with no work opened for weeks shows maintainers cannot afford working on a given subject

I agree, but last two weeks I was in PTO so really there has been only 4 working days elapsed since our last exchange on this PR, which is not a lot.

I understand but it's not only about your schedule, as PR author, but also about the maintainers of this repository and the team planning. Not mentioning the production risks: worth raising the topic as an issue for the infra since the infra team (including you) will have to plan this change and validate how to verify how and if it works as expected. Consequence of such a change are a large scope.

@lemeurherve
Copy link
Member Author

Closing as there is no immediate need for this feature.

@lemeurherve lemeurherve deleted the helpdesk4056-publishreports-failifempty branch May 30, 2024 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants