feat: first implementation of cdevents-sdk from json-schema #8

davidB · 2024-01-10T20:46:16Z

This PR is a WIP and a proposal implementation to have cdevents sdk for rust.

The PR contains:

typed struct for part of subject than json::Value (nearly any json type) or String without constraint
feedback from review
test: read and write examples from spec
enforce context.type to match the subject's type
cloudevents integration
~~CI (can be view in action at https://github.com/davidB/cdevents-sdk-rust/actions)~~
use enums, instead of string for field defined as enum in schema
test: generate random events and check against json schema

POSTPONED to another PR

documentation: Development, TODO, README (like other sdk)
test: setter/getter

I have a few other ideas, but after this PR

I need the rust sdk to continue my wip on davidB/cdviz: View software deployment's events & states

Let me know if I'm on the wrong path or if I should publish this library on my side.

PS: for the review, under the cdevents/src/generated folder, you can overlook just a few files. The generated folder is committed because rust crates are distributed as source, and I don't expect the user to build the generator and generate cdevents from git cloned spec before compiling the lib.

afrittoli · 2024-01-12T11:44:01Z

Hello @davidB, and thank you for your PR!
I see this is marked as draft - please let us know when this is ready for review.
I'm very new to rust but I'll do my best to review.

@rjtch has been working on the rust SDK a bit, so it would be great if the two of you could collaborate on the rust SDK.

FYI - we have public weekly CDEvents working group meetings (see https://github.com/cdevents/community?tab=readme-ov-file#how-to-get-involved) - if the time is convenient you're very welcome to join the meeting to discuss the rust SDK or any other topic around CDEvents.

afrittoli · 2024-01-12T11:46:18Z

@mlieberman85 FYI - in case you are interested in reviewing / contributing or even simply watching

davidB · 2024-01-14T18:47:37Z

@afrittoli Thank you, that motivate me to continue (I guess reviewer will be afraid by the volume of change in the PR, Sorry)

Before I start this PR, I looked at how to contact the community, but the slack channel is restricted to some email host, and I have a conflict on my agenda with the weekly meeting (and my contribution is out of working hours/resources).

mlieberman85 · 2024-01-14T19:00:48Z

I'm just watching this. I'm interested in replacing my generated structs here: https://github.com/kusaridev/skootrs/blob/main/src/model/cd_events/repo_created.rs that was made using a tool I wrote that calls out to https://github.com/oxidecomputer/typify, with something supported by the broader CD events community.

rjtch · 2024-01-15T08:38:35Z

@afrittoli Thank you, that motivate me to continue (I guess reviewer will be afraid by the volume of change in the PR, Sorry)

Before I start this PR, I looked at how to contact the community, but the slack channel is restricted to some email host, and I have a conflict on my agenda with the weekly meeting (and my contribution is out of working hours/resources).

Hey David, I can write you on slack if you want and we could work on this together. The community meeting is also conflicting with my agenda. I will contact you on slack.

afrittoli · 2024-01-15T08:49:50Z

@afrittoli Thank you, that motivate me to continue (I guess reviewer will be afraid by the volume of change in the PR, Sorry)

Before I start this PR, I looked at how to contact the community, but the slack channel is restricted to some email host, and I have a conflict on my agenda with the weekly meeting (and my contribution is out of working hours/resources).

Thanks David! Async collaboration is perfectly fine.
It would be good for you to join the slack nonetheless, it you don't mind could you try this link and let me know if it works? If not we'll need to have it fixed.

davidB · 2024-01-15T13:43:07Z

I'm on the slack workspace of the CD Foundation.
Thank you.

rjtch · 2024-01-15T14:41:03Z

I'm on the slack workspace of the CD Foundation. Thank you.

you can also join the #cdevents and the #general channels.

davidB · 2024-01-18T20:57:52Z

@afrittoli , @rjtch,
because there was no ci, lint when I forked the repository, I included it as part of this PR. And it will conflict with the stuff you are pushing.
The PR is in draft because it is not full (from the checklist), but you can look at it. So I can start to integrate your feedback or prepare for the coming discussion.

davidB · 2024-01-21T22:12:13Z

@afrittoli , @rjtch,

You can start the review; missing parts could added via the next PRs (smaller and more focused). Sorry for this too-big PR.

Maybe the next commits in this PR will be comments and documentation to explain some choices.

PS: I don't have permission to assign you as a reviewer, so I am notifying you via comment.

.github/workflows/mega-linter.yml

afrittoli · 2024-01-22T10:29:00Z

.github/workflows/mega-linter.yml

+  # (not recommended if you don't pay for GH Actions)
+  push:
+
+  # pull_request:


We should actually run on pull requests against main, to avoid new PRs breaking CI

with this configuration, it runs against every push (on every branch/pr). I can enable it for pull_request instead of push. But enabling both generates dual (overkill and sometimes conflicting) workflow.

The usual workflow is to push to a fork and create a PR from that. I may be wrong but I don't think that push to a fork would trigger the workflow on this repo.

The setup I usually use it pull_request only on main. If we at some point add branches to maintain specific releases we can extend the config from main only to whatever name pattern we have for release branches.

I made the change, but it will be visible into the coming PR with merged CI,...

.github/dependabot.yml

.editorconfig

afrittoli · 2024-01-22T10:36:16Z

.github/workflows/ci.yml

+        with:
+          fetch-depth: 0
+          submodules: "true"
+      - uses: jdx/rtx-action@v1


The repo is archived, so maybe not a good candidate? https://github.com/jdx/rtx-action

I'll check why he archive few days ago, maybe because the action could be replaced by 3 line of shell call.

rtx is simple and very useful tool (at least for me, when I have to switch between multiple project with various version of java, python, rust, node...)

Perhaps it's related to the rename rtx -> mise? https://dev.to/jdxcode/beginners-guide-to-rtx-ac4
I can see how rtx is very handy for the development environment on your laptop, but do we need it in CI jobs?

In my team, we found it useful to have the ci and local environment in sync.
I'll also a believer of "portable" CI, I mean every dev should be able to do the same as CI (if enough permission on third party system) or CI should just be like a developper that do every steps. So more the CI use same tooling than dev, better it is and less we are coupled to a CI system (trauma from migration between travis, azure pipeline, github-action, and hell of commit to debug/setup CI workflow special command).

And in the case of rtx+justfile/Makefile, it also provide a similar CI flow for various ecosystem.

Yes you're right it's due to rename ofrtx to mise https://mise.jdx.dev/rtx.html + some nice new feature.

@davidB i found mise also it is a good tool! the linter works now fine i think we could integrate mise in a next pr.

afrittoli · 2024-01-22T10:41:25Z

justfile

We already have a Makefile, so we should probably use that

the Makefile didn't exist when I forked. I'll try to convert the justfile into Makefile.

.gitmodules

.github/workflows/ci.yml

davidB · 2024-01-22T11:43:53Z

Thanks for the review, I'll try to do the change ASAP (maybe tomorrow).

Cargo.toml

README.md

afrittoli

Thanks for this, I started reviewing but it will take me a bit of extra time as I'm rather new to rust 🙏
That said, I don't want to slow down progress here, so if you feel confident this is a good way forward I'm happy to approve it (once the CI files part is sorted out).

cdevents-sdk/Cargo.toml

cdevents-sdk/examples/pipelinerun_finished.rs

cdevents-sdk/src/serde.rs

generator/README.md

afrittoli · 2024-01-22T12:35:52Z

Would you mind updating the PR title and description in the next iteration?

davidB · 2024-01-23T21:22:59Z

@rjtch I removed the ci + megalinter workflow from this PR (to work on the workflows) on an other PR (as suggested by @afrittoli). I also rebase the branch on master to resolve conflicts, switch to Makefile,... but now the PR is blocked by the new workflow:

lint commit message: => I'll rebase all commit to rephrase git message from gitmoji to conventional commit (the default setup of the plugin)
lint rust: the issue is more on the setup of the workflow, should I
- disable them until fixed (into an other PR)?
- ignore and the PR will be merged?
- fix them? (but it will defeat the purpose to work on CI into an other PR)

@afrittoli, What do you prefer "merge", "squash & merge", "rebase" when PR is ok?
Because if "squash & merge" I don't need to rephrase each commit message and just made use conventional commit for the squashed commit. It could also "it depends", on my side I mainly prefer "rebase" by default, "squash merge" (when I want to hide some intermediate commit, eg when fixing CI, debugging,...), forbid "merge" to have linear history on main branch.

Signed-off-by: David Bernard <[email protected]>

…generated samples - generate samples with arbitraries (proptest) Signed-off-by: David Bernard <[email protected]>

…ontext.type and subject.type Signed-off-by: David Bernard <[email protected]>

- avoid clippy genarte wrong warning about useless stuff - apply some clippy recommandation Signed-off-by: David Bernard <[email protected]>

Signed-off-by: David Bernard <[email protected]>

- clippy & tests need git submodules content Signed-off-by: David Bernard <[email protected]>

Signed-off-by: David Bernard <[email protected]>

davidB · 2024-01-30T21:02:37Z

@rjtch you can review the code and the few modifications, I made on Makefile, tools configuration, ci...

Sorry I rewrite the history several times (that will not ease the review as github could not remember what it already reviewed, and where comment should be inserted), but it was to please the new commit message rules (commitlint) and have a clean history when the PR will be rebase & merge.

rjtch · 2024-01-31T11:23:16Z

.github/workflows/linter.yaml

I used lint here in the matrix because i wanted to differentiate with next coming tests ci. Now that you renamed it to task it is not really clear what it is! If it is the linter_ci or the tests_ci.

Why not simply call this workflow ci (and ci.yml for the filename, github workflow often use this name, and the extension .yml, like what is generated by github and other framework).

We can discuss this point into a next PR. So I'll revert my change.

.mise.toml

Cargo.toml

Signed-off-by: David Bernard <[email protected]>

afrittoli · 2024-02-01T14:23:22Z

ide I mainly prefer "rebase" by default, "squash merge" (when I want to hide some intermediate commit, eg when fixing CI, debugging,...), forbid "merge" to have linear history on main branc

I also prefer rebase - I'll add "require linear history" to the branch protection rule.
I think it's fine to squash, but only problem I see is that GitHub won't re-run checks on the squashed commit, so the commit CI job won't actually do anything useful.

Alternatives are:

ask authors to squash locally and force push after approval
remove the commit linter
enable the GitHub merge queue, which, if I understand correctly, once something is squashed for merge, it will add it to a queue and retest (like tide or zuul have done for many years)

.mega-linter.yml

README.md

afrittoli · 2024-02-01T14:30:18Z

tools/cargo-deny/deny.toml

+# The unpublished packages (generator) would be ignored now
+# FIXME the generator is excluded explicitly (see Makefile), I don't know why this is not working
+[licenses.private]
+ignore = true


Can you create an issue in GitHub to track this?

#13 created, I don't found related issue on cargo-deny so maybe I misunderstood the purpose. It's why I didn't open yet an issue on there repository.

generator/src/main.rs

afrittoli

Thanks for this!

As a follow-up could you please add a CI job that runs the generator from the PR head and checks that there are no diff with the content of the PR?

Signed-off-by: David Bernard <[email protected]>

davidB · 2024-02-01T17:00:40Z

@afrittoli I'll not push change on CI in this PR to close it faster, but I include the makefile task to do the check that we could include in the next PR related to CI. I think other points are fixed

afrittoli

Thanks for the updates!

davidB force-pushed the newstart branch from f59d663 to d32ab57 Compare January 10, 2024 20:49

davidB force-pushed the newstart branch from 3e2b453 to 35ff6c4 Compare January 14, 2024 18:37

davidB force-pushed the newstart branch from 6431629 to ac67691 Compare January 18, 2024 20:06

davidB force-pushed the newstart branch from 54eeaeb to 36c3683 Compare January 18, 2024 21:51

davidB marked this pull request as ready for review January 21, 2024 22:07