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

feat!: add support for mulitple version of cdevents' specifications #19

Merged
merged 5 commits into from
Mar 24, 2024

Conversation

davidB
Copy link
Collaborator

@davidB davidB commented Mar 3, 2024

  • link as git submodule several versions of cdevents specs
  • add version into module name of subject's content
  • generate a module per specs with alias to the versionned subejct's content module
  • generate a module latest with alias to the latest version of each subject's content module
  • ignore subject'content with modifier in their version
  • update tests to load every json schema and test it (selection based on context.type

FIX #16

@davidB davidB mentioned this pull request Mar 3, 2024
@davidB davidB changed the title feat!: addsupport for mulitple version of cdevents' specifications feat!: add support for mulitple version of cdevents' specifications Mar 3, 2024
@davidB davidB requested review from afrittoli and rjtch March 3, 2024 12:59
rjtch

This comment was marked as duplicate.

cdevents-sdk/tests/specs.rs Show resolved Hide resolved
cdevents-sdk/tests/specs.rs Show resolved Hide resolved
@davidB davidB requested a review from rjtch March 3, 2024 15:55
davidB added 3 commits March 3, 2024 18:12
… spec

Signed-off-by: David Bernard <david.bernard.31@gmail.com>
- link as git submodule several versions of cdevents specs
- add version into module name of subject's content
- generate a module per specs with alias to the versionned subejct's content module
- generate a module `latest`  with alias to the latest version of each subject's content module
- ignore subject'content with modifier in their version
- update tests to load every json schema and test it (selection based on `context.type`

FIX #16

Signed-off-by: David Bernard <david.bernard.31@gmail.com>
Signed-off-by: David Bernard <david.bernard.31@gmail.com>
@davidB davidB force-pushed the feat_multi_version_support branch from 1549ef7 to f3f353b Compare March 3, 2024 17:13
Signed-off-by: David Bernard <david.bernard.31@gmail.com>
@davidB davidB force-pushed the feat_multi_version_support branch from 7aa5b5b to 9b1d7a3 Compare March 3, 2024 19:06
Signed-off-by: David Bernard <david.bernard.31@gmail.com>
@davidB davidB force-pushed the feat_multi_version_support branch from 9b1d7a3 to d38118f Compare March 3, 2024 19:06
Copy link
Contributor

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

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

Thanks for this! I like the idea of the multiple submodules.
I left some comments - it looks like the modules generated from the main version of the spec are not up to date. And we should not release them as part of the SDK in any case.

Comment on lines +1 to +7
[submodule "cdevents-specs/spec-v0.3"]
path = cdevents-specs/spec-v0.3
url = https://github.com/cdevents/spec.git
branch = spec-v0.3
[submodule "cdevents-specs/main"]
path = cdevents-specs/main
url = https://github.com/cdevents/spec.git
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea of having multiple submodules, we might do the same for other SDKs.
I'm not entirely sure about the main one - I think it's fine as long as there is a way to exclude it from a release. Events on main in the spec have a -draft version and are not meant to be used until they are released, so we should not release an SDK that supports producing them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the generator exclude every json with "draft" or any modifier in the version fragment from context.type.


use serde::{Serialize, Deserialize};
pub mod latest {
Copy link
Contributor

Choose a reason for hiding this comment

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

While it's nice to generate latest for dev preview sake, we should not include it in a release

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the purpose of latest is not for dev preview, it's to ease the development of library.

scenario for "latest":

  • inside the code, the developer (user of the SDK) uses the "latest" module
  • when the version of the SDK is upgraded, the compiler will raise an error for incompatible event structure, then the developer can
    • update the code to match the new structure
    • OR change "latest" to a specific "spec module"
    • OR change from the "latest" module to the parent and use the per version event.

By using "latest" module, developers can easily upgrade to the latest version of each event, maybe by changing the SDK's version number. (The "latest" module behaves similarly to the behavior in other SDKs or currently on the main branch.)

For developer that don't use the "latest" module, then upgrading means changing the dependencies and do not forgot to change each event version (via per spec module, or each one) inside the code. So maybe a wrong feeling of upgrade.

The "latest" scenario is also the one most compatible with tool like dependabot, renovate,...

use serde::{Serialize, Deserialize};
pub mod latest {
pub use super::artifact_packaged_0_1_1 as artifact_packaged;
pub use super::artifact_published_0_1_1 as artifact_published;
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be worth checking the generator logic - on main the artifact published event is on version 0.2.0-draft (https://github.com/cdevents/spec/blob/main/schemas/artifactpublished.json)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As I mention "-draft" are not generated (too unstable, and also confusing in term of naming modules,...)

@davidB
Copy link
Collaborator Author

davidB commented Mar 7, 2024

Thanks for this! I like the idea of the multiple submodules. I left some comments - it looks like the modules generated from the main version of the spec are not up to date. And we should not release them as part of the SDK in any case.

The issue with "main" is that we need to update the git submodule to have the latest version.

@davidB davidB merged commit b778dd5 into main Mar 24, 2024
7 checks passed
@davidB davidB deleted the feat_multi_version_support branch January 7, 2025 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: No status
Development

Successfully merging this pull request may close these issues.

support multiple version of specifications
3 participants