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

Follow up 126: factor out hardcoded common name #134

Merged
merged 6 commits into from
Jun 22, 2019

Conversation

mikaelarguedas
Copy link
Member

Factors out the hardcoded sros2testCA into a constant DEFAULT_COMMON_NAME.

Once this is merged, the tests checking for the common name can be updated to leverage this new constant as well.

Long term, this should likely be exposed to users so that they can pick the name used instead of this hardcoded one

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

@kyrofa kyrofa left a comment

Choose a reason for hiding this comment

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

Thank you, this is a welcome refactor. Curious to get your thoughts regarding the public/private API.

sros2/sros2/api/__init__.py Outdated Show resolved Hide resolved
sros2/sros2/api/__init__.py Outdated Show resolved Hide resolved
@kyrofa
Copy link
Member

kyrofa commented Jun 20, 2019

Once this is merged, the tests checking for the common name can be updated to leverage this new constant as well.

Is there a reason not to do that here?

@mikaelarguedas
Copy link
Member Author

Is there a reason not to do that here?

There were (still are?) ongoing PRs adding tests or touching tests using of the hard-coded common name when I wrote these commits. So I'd figure we'll let things settle a bit to then do a full search replace.

@kyrofa
Copy link
Member

kyrofa commented Jun 20, 2019

So I'd figure we'll let things settle a bit to then do a full search replace.

Fair point, as long as we don't forget! 👍

This reverts commit 3a45a75.

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
@mikaelarguedas
Copy link
Member Author

Fair point, as long as we don't forget! +1

Opened #137 so that we don't 👍

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

@kyrofa kyrofa left a comment

Choose a reason for hiding this comment

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

Thank you!

@jacobperron
Copy link
Member

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@jacobperron jacobperron merged commit 49d63a5 into ros2:master Jun 22, 2019
@mikaelarguedas mikaelarguedas deleted the follow-up-126 branch June 22, 2019 02:16
ruffsl pushed a commit to ruffsl/sros2 that referenced this pull request Aug 6, 2019
* use upper case names for constants

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

* factor out hardcoded common name

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

* avoid using python builtins as argument names

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

* Revert "use upper case names for constants"

This reverts commit 3a45a75.

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

* format cnf string to be more readable

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

* make DEFAULT_COMMON_NAME private

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
Signed-off-by: ruffsl <roxfoxpox@gmail.com>
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