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

fixed commit #10

Closed
wants to merge 10 commits into from
Closed

fixed commit #10

wants to merge 10 commits into from

Conversation

rjtch
Copy link
Contributor

@rjtch rjtch commented Jan 18, 2024

installed and caches steps for rust and rust

@rjtch
Copy link
Contributor Author

rjtch commented Jan 18, 2024

This is in WiP and will be able to be merged soon.

Cargo.toml Outdated Show resolved Hide resolved
@rjtch
Copy link
Contributor Author

rjtch commented Jan 20, 2024

Sorrry @afrittoli i will rework it i changed everything because of this message ´taiki-e/install-action@protoc, dtolnay/rust-toolchain@beta, and swatinem/rust-cache@v2 are not allowed to be used in cdevents/sdk-rust. Actions in this workflow must be: within a repository owned by cdevents, created by GitHub, or matching the following: wagoid/commitlint-github-action@*.´ https://github.com/cdevents/sdk-rust/actions/runs/7576295830.

@afrittoli
Copy link
Contributor

Sorrry @afrittoli i will rework it i changed everything because of this message ´taiki-e/install-action@protoc, dtolnay/rust-toolchain@beta, and swatinem/rust-cache@v2 are not allowed to be used in cdevents/sdk-rust. Actions in this workflow must be: within a repository owned by cdevents, created by GitHub, or matching the following: wagoid/commitlint-github-action@*.´ https://github.com/cdevents/sdk-rust/actions/runs/7576295830.

I can enable any action we need in the repo config, by default non-GitHub-maintained actions are disallowed for security reasons.

@afrittoli
Copy link
Contributor

Thanks for the latest updates, it's looking good!

I think for the commit check to pass it would be easier if you squashed all commits to one, and updated the commit message. It would be nice if you could set the PT title and description to something with a bit more context too 🙏

@rjtch
Copy link
Contributor Author

rjtch commented Jan 26, 2024

Thanks for the latest updates, it's looking good!

I think for the commit check to pass it would be easier if you squashed all commits to one, and updated the commit message. It would be nice if you could set the PT title and description to something with a bit more context too 🙏

yes sure i will

Comment on lines 47 to 58
- name: Install rust toolchain
uses: dtolnay/rust-toolchain@stable
uses: actions-rust-lang/setup-rust-toolchain@v1
with:
profile: minimal
toolchain: nightly
components: clippy, rustfmt
- name: Install development tools
uses: taiki-e/install-action@protoc
uses: taiki-e/install-action@v2
with:
tool: just, cargo-deny, dprint
tool: cargo-deny, dprint
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we could switch to mise (formerly rtx) to handle the installation on local env and on ci?

(in this PR or the next one)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By default cd-event resctrict the use of public actions so they have to be added after some inspection. I use this to have as small image size as possible.

Comment on lines 3 to 5
"**/*.toml",
"**/*.yaml",
"**/*.rs"
Copy link
Collaborator

Choose a reason for hiding this comment

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

warning: with prettiers we should also provide the config for IDE/Editor to be in sync or for other linter, formatters. By example:

  • print didn't agree with my VsCode auto-formatter (BetterTOML + EditorConfig) on toml
  • rust code is formatted with cargo fmt (default rules)
  • If we use megalinter/superlinter they already provide a bunch of yaml, json,... linter)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my problem with mega-superlinter is the size of the image which are too big. One thing we could do would be to use prettier as pre-commit for linter all files and then remove this.

Comment on lines 13 to 15
"Apache-2.0",
"MIT",
"BSD-2-Clause",
Copy link
Collaborator

Choose a reason for hiding this comment

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

For my PR, more licenses will be needed (like CC0-1.0) some by lib used by the generator, some by delivered sdk.

Do you think, we should use the same rules for both (as the generator is not distributed)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes but i don't know actually how the cd-event team would like to manage linsences but for me it is not a problem.

rjtch and others added 6 commits January 27, 2024 18:01
Feat/add linter ci check cdevents#2 (cdevents#7)

* added apache 2.0 license to all crates
* semgrep and dprint for code formatting
* cargo-deny for managing crates licenses

Signed-off-by: rjtch <[email protected]>
* added apache 2.0 license to all crates
* semgrep and dprint for code formatting
* cargo-deny for managing crates licenses

Signed-off-by: Hergy Fongue <[email protected]>
* added apache 2.0 license to all crates
* semgrep and dprint for code formatting
* cargo-deny for managing crates licenses

Signed-off-by: Hergy Fongue <[email protected]>
Signed-off-by: rjtch <[email protected]>
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.

3 participants