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

Corrected a problem with the acceptance test action. #1583

Merged
merged 1 commit into from
Sep 19, 2023

Conversation

jcpitre
Copy link
Contributor

@jcpitre jcpitre commented Sep 19, 2023

acceptance_test.yml did not run properly.
It seems the replacement for ::set-output (i.e. >> $GITHUB_OUTPUT) does not accept newlines in the data.
This was a problem with multi-line commit messages.
Just replaced newlines by spaces.

It seems the replacement for ::set-output (i.e. >> $GITHUB_OUTPUT) does not accept newlines in the data.
# Obtain the last commit from the branch to merge (hence the HEAD^2).
# In case of multi-line commit messages, remove any \n, because the GITHUB_OUTPUT method
# of sending data to other jobs does not like them.
run: echo "pr_commit_message=\"$(git log --format=%B -n 1 HEAD^2 | tr '\n' ' '))\"" >> $GITHUB_OUTPUT
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tried like this?

echo "pr_commit_message=$(git log --format=%B -n 1 HEAD | tr '\n' ' '))"

... because in terminal, it fails with the ^2. And the extra "" are not necessary, I've confirmed this.

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 HEAD^2 works for a merge. It takes the previous message from the branch that will be merged.
HEAD for a merge would be the merge message itself (Merge branch ... into ...)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I didn't know that. J'va me coucher moins niaiseux ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the quotes.

@github-actions
Copy link
Contributor

✅ Rule acceptance tests passed.
New Errors: 1 out of 1478 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
Dropped Errors: 0 out of 1478 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
New Warnings: 0 out of 1478 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
Dropped Warnings: 0 out of 1478 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
0 out of 1478 sources (~0 %) are corrupted.
Commit: d10cdad
Download the full acceptance test report here (report will disappear after 90 days).
✅ Rule acceptance tests passed.

@fredericsimard fredericsimard merged commit 528ac92 into master Sep 19, 2023
333 checks passed
@fredericsimard fredericsimard deleted the repair_acceptance_test branch September 19, 2023 20:34
@jcpitre
Copy link
Contributor Author

jcpitre commented Sep 19, 2023

One last commit was not picked up.

@fredericsimard
Copy link
Contributor

fredericsimard commented Sep 19, 2023

One last commit was not picked up.

@jcpitre Do you mean this line?

run: echo "pr_commit_message=\"$(git log --format=%B -n 1 HEAD^2 | tr '\n' ' '))\"" >> $GITHUB_OUTPUT

Removing the double quotes?

@jcpitre jcpitre mentioned this pull request Sep 19, 2023
@jcpitre
Copy link
Contributor Author

jcpitre commented Sep 19, 2023

Check #1584

@fredericsimard
Copy link
Contributor

Approved it.

Copy link
Contributor

@qcdyx qcdyx left a comment

Choose a reason for hiding this comment

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

LGTM - it solved the problem I met.

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.

4 participants