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

[improve][ci] Improve CI ssh access in forks, don't fail build if setting up ssh access fails #19127

Merged
merged 1 commit into from
Jan 5, 2023

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Jan 4, 2023

Motivation

The ssh access setup step that is part of Pulsar CI can fail in builds that are run in forks. The ssh access feature is active in CI builds run in forks to allow connecting to the running build with ssh for debugging purposes.

This is the ssh access setup step:
https://github.com/apache/pulsar/blob/master/.github/workflows/pulsar-ci.yaml#L88-L93

Sometime this fails with an error message "flag needs an argument: --admin-socket". Example of failure:
https://github.com/michaeljmarshall/pulsar/actions/runs/3833202322/jobs/6527659487#step:4:150

Modifications

  • wait up to 10 seconds for admin socket to appear
  • add "continue-on-error: true" to ssh setup step so that errors don't prevent the build from continuing

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: lhotari#122

…ting up ssh access fails

- address error message "flag needs an argument: --admin-socket"
  - wait up to 10 seconds for admin socket to appear
- add "continue-on-error: true" to ssh setup step so that errors don't prevent the build from continuing
@lhotari lhotari force-pushed the lh-improve-ci-ssh-access-in-forks branch from 0ca1168 to d56a9c1 Compare January 4, 2023 07:05
@lhotari
Copy link
Member Author

lhotari commented Jan 4, 2023

/pulsarbot rerun-failure-checks

@codecov-commenter
Copy link

codecov-commenter commented Jan 4, 2023

Codecov Report

Merging #19127 (d56a9c1) into master (9ec1d07) will decrease coverage by 0.38%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #19127      +/-   ##
============================================
- Coverage     47.73%   47.34%   -0.39%     
+ Complexity    10819    10716     -103     
============================================
  Files           712      712              
  Lines         69645    69645              
  Branches       7481     7481              
============================================
- Hits          33242    32975     -267     
- Misses        32699    32963     +264     
- Partials       3704     3707       +3     
Flag Coverage Δ
unittests 47.34% <ø> (-0.39%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../pulsar/broker/service/SharedConsumerAssignor.java 3.70% <0.00%> (-64.82%) ⬇️
...rvice/schema/KeyValueSchemaCompatibilityCheck.java 21.62% <0.00%> (-45.95%) ⬇️
...apache/pulsar/broker/service/EntryAndMetadata.java 0.00% <0.00%> (-40.75%) ⬇️
...r/service/AbstractDispatcherMultipleConsumers.java 56.45% <0.00%> (-14.52%) ⬇️
.../apache/pulsar/broker/admin/impl/PackagesBase.java 54.12% <0.00%> (-13.77%) ⬇️
...e/pulsar/broker/service/EntryBatchIndexesAcks.java 82.14% <0.00%> (-10.72%) ⬇️
...sistent/PersistentDispatcherMultipleConsumers.java 51.96% <0.00%> (-7.18%) ⬇️
...ersistentStickyKeyDispatcherMultipleConsumers.java 54.41% <0.00%> (-5.89%) ⬇️
...pulsar/broker/service/PulsarCommandSenderImpl.java 67.85% <0.00%> (-5.62%) ⬇️
...e/HashRangeExclusiveStickyKeyConsumerSelector.java 36.92% <0.00%> (-3.08%) ⬇️
... and 37 more

@lhotari lhotari requested a review from tisonkun January 5, 2023 08:01
Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

LGTM.

@tisonkun
Copy link
Member

tisonkun commented Jan 5, 2023

Merging...

@tisonkun tisonkun merged commit 4028ad3 into apache:master Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ci doc-not-needed Your PR changes do not impact docs ready-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants