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

Fixes #8985: Add node key in managed_nodes system variable #1226

Conversation

fanf
Copy link
Member

@fanf fanf commented Oct 1, 2016

@fanf
Copy link
Member Author

fanf commented Oct 1, 2016

PR rebased

@fanf fanf force-pushed the arch_8985/add_node_key_in_managed_nodes_system_variable branch from ef5d002 to 8b3ef23 Compare October 1, 2016 22:24
@fanf
Copy link
Member Author

fanf commented Oct 1, 2016

PR rebased

@fanf fanf force-pushed the arch_8985/add_node_key_in_managed_nodes_system_variable branch from 8b3ef23 to e53fb08 Compare October 1, 2016 22:28
Copy link
Member

@ncharles ncharles left a comment

Choose a reason for hiding this comment

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

This looks great.
However, as you stated, there are a lot of casting - and I feel you should guard them around a try { } block


//the parser able to read PEM files
val parser = new PEMParser(new StringReader(key.value))

Copy link
Member

Choose a reason for hiding this comment

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

There are a lot of asInstanceOf - shouldn't you wrap it around a try to catch casting exception ?

Copy link
Member Author

Choose a reason for hiding this comment

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

They are already with tryo - which one did I missed ?

Copy link
Member

Choose a reason for hiding this comment

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

huh - i didn't see any of them ... don't know why
It does look ok then

Copy link
Member

@ncharles ncharles left a comment

Choose a reason for hiding this comment

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

This looks great, thank you

@Normation-Quality-Assistant
Copy link
Contributor

OK, merging this PR

@Normation-Quality-Assistant Normation-Quality-Assistant merged commit e53fb08 into Normation:master Oct 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants