Skip to content

feat(CI): handle remote files in a safer way#2217

Merged
shamardy merged 4 commits intodevfrom
safer-pipelines
Sep 27, 2024
Merged

feat(CI): handle remote files in a safer way#2217
shamardy merged 4 commits intodevfrom
safer-pipelines

Conversation

@onur-ozkan
Copy link
Copy Markdown

This PR updates GHA runners to use locked scripts rather than always using the latest one from the master branch. It also adds a new GHA helper/plugin to easily download files and verify their checksums.

cc @Alrighttt

Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

  • name: Install libudev (Linux)
  • name: Install python3
  • name: Install paramiko pip package

Is there some reason we don't want to lock these as well?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Is there some reason we don't want to lock these as well?

They are fairly known dependencies and if we lock the versions we might miss the important fixes. It's also helpful to see if we are compatible with their recent releases.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can we use scp instead of paramiko? I understand windows is an outlier here, and we might continue to use it in that case.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It's not clear to me what requires libudev. It's reference in the fmt-and-lint workflow only. Is it required for cargo clippy or cargo fmt?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Can we use scp instead of paramiko? I understand windows is an outlier here, and we might continue to use it in that case.

We can but we need to rewrite this script.

It's not clear to me what requires libudev. It's reference in the fmt-and-lint workflow only. Is it required for cargo clippy or cargo fmt?

It's probably outdated (since e71e2cf maybe?), let me try to remove it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Author

@onur-ozkan onur-ozkan Sep 24, 2024

Choose a reason for hiding this comment

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

Most probably required from the non-default features.

Reverted.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

requirement is introduced by solana-remote-wallet@1.9.20

Could we check if this is absolutely necessary? Can someone please confirm that we actually use this crate?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Could we check if this is absolutely necessary? Can someone please confirm that we actually use this crate?

It is necessary for enable-solana feature (and we run --all-features for linting runner). As we don't provide any output (like executable binary) to public from linting runner, there shouldn't be any concern for including libudev in that runner I think.

mariocynicys
mariocynicys previously approved these changes Sep 26, 2024
Copy link
Copy Markdown
Collaborator

@mariocynicys mariocynicys left a comment

Choose a reason for hiding this comment

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

LGTM :)

Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
@shamardy shamardy merged commit 6dc3c7d into dev Sep 27, 2024
@shamardy shamardy deleted the safer-pipelines branch September 27, 2024 09:33
dimxy pushed a commit that referenced this pull request Oct 4, 2024
* dev:
  fix(orders): fix cancel order race condition using time-based cache (#2232)
  fix(legacy-swap): taker failed spend maker payment marked as failed (#2199)
  chore(adex-cli): deprecate adex-cli (#2234)
  feat(new-RPC): connection healthcheck implementation for peers  (#2194)
  fix(proxy-signature): add message lifetime overflows (#2233)
  feat(CI): handle remote files in a safer way (#2217)
  chore(doc): update issue address in README (#2227)
  fix(merge): remove duplicated db_root function (#2229)
  feat(wallets): add `get_wallet_names` rpc (#2202)
  chore(tests): don't use `.wait()` and use `block_on` instead (#2220)
  fix(native-rpc): remove escaped response body (#2219)
  fix(clippy): fix coins mod clippy warnings in wasm (#2224)
  feat(core): handling CTRL-C signal with graceful shutdown (#2213)
  docs(README): fix typos (#2212)
  remove the non-sense arguments (#2216)
  fix(db): stop creating the all-zeroes dir on KDF start (#2218)
dimxy pushed a commit that referenced this pull request Oct 4, 2024
* dev:
  fix(orders): fix cancel order race condition using time-based cache (#2232)
  fix(legacy-swap): taker failed spend maker payment marked as failed (#2199)
  chore(adex-cli): deprecate adex-cli (#2234)
  feat(new-RPC): connection healthcheck implementation for peers  (#2194)
  fix(proxy-signature): add message lifetime overflows (#2233)
  feat(CI): handle remote files in a safer way (#2217)
  chore(doc): update issue address in README (#2227)
  fix(merge): remove duplicated db_root function (#2229)
  feat(wallets): add `get_wallet_names` rpc (#2202)
  chore(tests): don't use `.wait()` and use `block_on` instead (#2220)
  fix(native-rpc): remove escaped response body (#2219)
  fix(clippy): fix coins mod clippy warnings in wasm (#2224)
  feat(core): handling CTRL-C signal with graceful shutdown (#2213)
  docs(README): fix typos (#2212)
  remove the non-sense arguments (#2216)
  fix(db): stop creating the all-zeroes dir on KDF start (#2218)
dimxy pushed a commit that referenced this pull request Oct 17, 2024
* dev:
  fix(cosmos): fix tx broadcasting error (#2238)
  chore(solana): remove solana implementation (#2239)
  chore(cli): remove leftover subcommands from help message (#2235)
  fix(orders): fix cancel order race condition using time-based cache (#2232)
  fix(legacy-swap): taker failed spend maker payment marked as failed (#2199)
  chore(adex-cli): deprecate adex-cli (#2234)
  feat(new-RPC): connection healthcheck implementation for peers  (#2194)
  fix(proxy-signature): add message lifetime overflows (#2233)
  feat(CI): handle remote files in a safer way (#2217)
  chore(doc): update issue address in README (#2227)
  fix(merge): remove duplicated db_root function (#2229)
  feat(wallets): add `get_wallet_names` rpc (#2202)
  chore(tests): don't use `.wait()` and use `block_on` instead (#2220)
  fix(native-rpc): remove escaped response body (#2219)
  fix(clippy): fix coins mod clippy warnings in wasm (#2224)
  feat(core): handling CTRL-C signal with graceful shutdown (#2213)
  docs(README): fix typos (#2212)
  remove the non-sense arguments (#2216)
  fix(db): stop creating the all-zeroes dir on KDF start (#2218)
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