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

App manifest v3 #95

Merged
merged 2 commits into from
Aug 18, 2023
Merged

App manifest v3 #95

merged 2 commits into from
Aug 18, 2023

Conversation

MP91
Copy link
Contributor

@MP91 MP91 commented Aug 18, 2023

Describe your changes

Updates the AppManifest in all examples to v3

Issue ticket number and link

Checklist - Manual tasks

  • Examples are executing successfully
  • Created/updated unit tests. Code Coverage percentage on new code shall be >= 80%.
  • Created/updated integration tests.
  • Devcontainer can be opened successfully
  • Devcontainer can be opened successfully behind a corporate proxy
  • Devcontainer can be re-built successfully
  • Extended the documentation (e.g. README.md, CONTRIBUTING.md, Velocitas)

Copy link
Contributor

@kse3hi kse3hi 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
Contributor

@kse3hi kse3hi left a comment

Choose a reason for hiding this comment

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

Have you checked if all of them are able to be run?

@MP91
Copy link
Contributor Author

MP91 commented Aug 18, 2023

Have you checked if all of them are able to be run?

Yes, at least all are starting correctly without any errors.

@MP91 MP91 merged commit 0030f44 into main Aug 18, 2023
@MP91 MP91 deleted the appManifest_v3 branch August 18, 2023 06:36
Copy link
Member

@BjoernAtBosch BjoernAtBosch left a comment

Choose a reason for hiding this comment

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

Some finding(s) - even if already released ...

Comment on lines +20 to +30
{
"type": "grpc-interface",
"config": {
"src": "https://github.com/eclipse/kuksa.val/blob/master/kuksa_databroker/proto/sdv/databroker/v1/broker.proto",
"required": {
"methods": [
"Subscribe"
]
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I thought, this should go out. Databroker interface is "implicitly" defined by referencing the VSS model.
@doosuu: Or?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I was not aware of this discussion, but in the concept nothing is written about that...

Copy link
Member

Choose a reason for hiding this comment

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

Yes, databroker is not an explicit dependency.

{
"type": "grpc-interface",
"config": {
"src": "https://github.com/eclipse/kuksa.val/blob/master/kuksa_databroker/proto/sdv/databroker/v1/broker.proto",
Copy link
Member

Choose a reason for hiding this comment

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

In case that we keep the databroker interface "referencing" here (see my other finding), shouldn't we specify a fixed version of the proto source (instead of "master")?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Master just refers to the repository. I expect the v1 in "proto/sdv/databroker/v1/broker.proto" to be updated if something in the interface changes

Copy link
Member

Choose a reason for hiding this comment

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

Yes and no: "v1" is just a major version, whose purpose is to allow to maintain multiple major versions in parallel.
A version "v2" is added once some incompatible change is done in the interface. As long as there are only compatible changes the interface is still maintained as "v1". (There are no minor or patch versions at this level.)
Function-wise this should not harm, as the app still uses the "old" interface (and exactly specifies the methods used). But the advantage of pinning the exact version would be reproducibility. Maybe this is more relevant for C++, because if a new compatible version is released on master ("under the hood" of v1), this will end up in re-compilation.

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