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

Try to fix CI sctp test. #19511

Closed
wants to merge 1 commit into from
Closed

Conversation

slontis
Copy link
Member

@slontis slontis commented Oct 27, 2022

For some reason when I tried this before it worked. So retrying this again to see if it still works.
Not sure why it decided to fail once merged yet.

Checklist
  • documentation is added or updated
  • tests are added or updated

@slontis
Copy link
Member Author

slontis commented Oct 27, 2022

This is a test -(mainly for my own sanity)

@slontis
Copy link
Member Author

slontis commented Oct 27, 2022

@paulidale - this verifies what we saw before.. i.e. it works here... Maybe that means the infrastructure is different for the daily build?

Copy link
Contributor

@paulidale paulidale left a comment

Choose a reason for hiding this comment

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

I can't see a difference. Maybe add this to ci.yml instead?

run: |
sudo apt-get update
sudo apt-get -yq install lksctp-tools libsctp-dev
checksctp
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be enough and get rid of the OK environment variable:

- name: Check SCTP is available
  run: checksctp

Copy link
Member Author

Choose a reason for hiding this comment

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

Except for when it fails again on merge and I have to spend another day looking at it :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that. This step will fail if checksctp does and the rest won't run. It avoid the conditionals.

Copy link
Member Author

Choose a reason for hiding this comment

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

We might have to try this out by merging..


- name: make test
run: make test HARNESS_JOBS=${HARNESS_JOBS:-4}
if: $OK == 0
Copy link
Contributor

Choose a reason for hiding this comment

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

New line at end of file.

@slontis slontis force-pushed the sctp_ci_attempt2 branch 2 times, most recently from 333ac71 to 24700b8 Compare October 27, 2022 06:12
@slontis
Copy link
Member Author

slontis commented Oct 27, 2022

@paulidale - just entered something bad to test it out.. That skips the tests and marks it as green

@slontis
Copy link
Member Author

slontis commented Oct 27, 2022

What I am trying to do here is skip doing the tests if sctp cant be setup, and report a green result..
I would like to try merging this so we can see what happens.. It will still be better than failing as it does currently in the daily build.
If we see that is is still always skipping then we can take it back out again.

For some reason the newly introduced CI test
for sctp causes issues. It is unknown why this
seems to work when testing, but doesnt work
once it was merged.
The test has been put into its own file, with
skips on error if the setup fails..
This will need to be merged to test if this
works.
@t8m t8m added branch: master Merge to master branch triaged: bug The issue/pr is/fixes a bug labels Oct 27, 2022
@paulidale paulidale added approval: review pending This pull request needs review by a committer severity: urgent Fixes an urgent issue (exempt from 24h grace period) labels Oct 27, 2022
@slontis
Copy link
Member Author

slontis commented Nov 2, 2022

ping

Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

Let's try merging this to see.

@t8m t8m added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: review pending This pull request needs review by a committer labels Nov 2, 2022
@t8m
Copy link
Member

t8m commented Nov 2, 2022

Merging as this is CI failure fix.

@t8m
Copy link
Member

t8m commented Nov 2, 2022

Merged to master branch. Thank you.

@t8m t8m closed this Nov 2, 2022
openssl-machine pushed a commit that referenced this pull request Nov 2, 2022
For some reason the newly introduced CI test
for sctp causes issues. It is unknown why this
seems to work when testing, but doesnt work
once it was merged.
The test has been put into its own file, with
skips on error if the setup fails..
This will need to be merged to test if this
works.

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #19511)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
For some reason the newly introduced CI test
for sctp causes issues. It is unknown why this
seems to work when testing, but doesnt work
once it was merged.
The test has been put into its own file, with
skips on error if the setup fails..
This will need to be merged to test if this
works.

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#19511)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch severity: urgent Fixes an urgent issue (exempt from 24h grace period) triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants