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

add reference to schema in generated permission files #84

Merged
merged 1 commit into from
Feb 23, 2019

Conversation

mikaelarguedas
Copy link
Member

This PR adds a reference to the schema used in the generated xml files (similar to what is in the default governance file) so that users can perform validation on the files.

@ruffsl It looks like this repo now has a copy of the permission and governance XSDs from the OMG website. Is the expected way of managing schema to commit new copies of the schemas to this repo when a new DDS Security version comes out?

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
@mikaelarguedas mikaelarguedas added in progress Actively being worked on (Kanban column) in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Feb 21, 2019
@ruffsl
Copy link
Member

ruffsl commented Feb 21, 2019

Is the expected way of managing schema to commit new copies of the schemas to this repo when a new DDS Security version comes out?

I'd expect so. I don't know of another way the CLI could acquire the DDS schema without it being pre-installed, other than requiring users have constant internet access to the public internet 🌏 . Can we get it from the vendor installed?

@mikaelarguedas
Copy link
Member Author

Can we get it from the vendor installed?

I remember that at least RTI and Fast-RTPS provide a version of these, not sure about Opensplice or other vendors.
Potential alternatives:

  • Use the one provided by the vendors (though it assumes they provide the same version, which was not true in the past)
  • Fetch the schema from the OMG website at sros2 build time and install it, this way we dont need to embed a copy here or a valid internet connexion when using the CLI (though we'll likely need to update the download URL when a new version comes out).
  • Host them on http://download.ros.org so that they have an http URL (some tools like xmllint can't fetch https hosted schemas), this could potentially allow us to keep the same URL.

I have nothing against the current solution about embedding it in the repo, just brainstorming if we could come up with anything avoiding duplication

@ruffsl
Copy link
Member

ruffsl commented Feb 21, 2019

Fetch the schema from the OMG website at sros2 build time and install it, this way we dont need to embed a copy here or a valid internet connexion when using the CLI (though we'll likely need to update the download URL when a new version comes out).

That sounds nice! Could we add a bit of logic to the setup.py to first check the source folder for it before downloading a version from OMG as a fall back. This would keep it easy for developers to use custom schemas, or test unreleased schemas in beta.

some tools like xmllint can't fetch https hosted schemas

The fact the no XML template processor can use HTTPS was infuriating when I was prototyping! If we do scrape the XSD from OMG at build time, lets makes sure if via https though.

I have nothing against the current solution about embedding it in the repo, just brainstorming if we could come up with anything avoiding duplication

The XSD hasn't changed often TBH, I think the latest release merely added and optional <script> element that I couldn't find in the documentation.

@mikaelarguedas
Copy link
Member Author

Pulling it at build time sounds good to me. This will likely be a separate PR though as this one is just for consistency accross governance/permission xml files and allow end consumers to have a self-contained permission files.

I would advocate to get this in as is as I don't know if/when I'll have time to work on the next step.

The XSD hasn't changed often TBH, I think the latest release merely added and optional <script> element that I couldn't find in the documentation.

Yeah last change I remember was the renaming of allow_unauthenticated_join vs allow_unauthenticated_participants.

Copy link
Member

@ruffsl ruffsl left a comment

Choose a reason for hiding this comment

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

LGTM

@ruffsl
Copy link
Member

ruffsl commented Feb 22, 2019

I would advocate to get this in as is as I don't know if/when I'll have time to work on the next step.

Feel free to merge when ready. Could you open a ticket for this so we don't forget.

@mikaelarguedas
Copy link
Member Author

Feel free to merge when ready. Could you open a ticket for this so we don't forget.

👍 #86

@mikaelarguedas mikaelarguedas merged commit dace0b5 into ros2:master Feb 23, 2019
@mikaelarguedas mikaelarguedas removed the in review Waiting for review (Kanban column) label Feb 23, 2019
@mikaelarguedas mikaelarguedas deleted the add_schema_reference branch February 23, 2019 00:40
mjcarroll added a commit that referenced this pull request Mar 8, 2019
* Correct sros2 cli test folder location (#83)

* Update test folder location
fixing incomplete rebase from #72

* Remove old yaml profile examples
fixing incomplete rebase from #72

* add reference to schema in generated permission files (#84)

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

* Add missing attributes to test permissions XML file

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* fix status print to match commands invoked

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

* Fix bug preventing generate_policy verb from working with publishers and services

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Add CMake lint test to sros2_cmake (#90)

Fixed lint errors accordingly.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
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.

2 participants