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

IR extension for hierarchical attributes and variables #17

Merged
merged 6 commits into from
Mar 9, 2023

Conversation

joaotgoncalves
Copy link
Collaborator

No description provided.

glitch/repr/inter.py Outdated Show resolved Hide resolved
@jff jff requested a review from Nfsaavedra February 10, 2023 21:04
glitch/repr/inter.py Outdated Show resolved Hide resolved
glitch/parsers/cmof.py Outdated Show resolved Hide resolved
glitch/parsers/cmof.py Outdated Show resolved Hide resolved
glitch/parsers/cmof.py Outdated Show resolved Hide resolved
glitch/parsers/cmof.py Outdated Show resolved Hide resolved
glitch/parsers/cmof.py Outdated Show resolved Hide resolved
Copy link
Member

@Nfsaavedra Nfsaavedra left a comment

Choose a reason for hiding this comment

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

I think you should add some test that considers the case being handled here. Maybe it is a good time to add the first unit test for the parsing portion. I am not sure if the integration tests cover this case for every technology. Also, please add a more descriptive title and if necessary a description to the PR. It can be valuable for future contributions. Thanks!

glitch/analysis/security.py Outdated Show resolved Hide resolved
glitch/parsers/cmof.py Outdated Show resolved Hide resolved
glitch/parsers/cmof.py Outdated Show resolved Hide resolved
glitch/parsers/cmof.py Outdated Show resolved Hide resolved
glitch/parsers/cmof.py Show resolved Hide resolved
jff
jff previously requested changes Feb 10, 2023
Copy link
Member

@jff jff left a comment

Choose a reason for hiding this comment

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

I left some comments. See also @Nfsaavedra's comments.

@Nfsaavedra Nfsaavedra changed the title Ir extension IR extension for hierarchical attributes and variables Feb 17, 2023
@Nfsaavedra Nfsaavedra added the enhancement New feature or request label Feb 17, 2023
… tests for hierarchical variables/attributes cases; and new Superclass for attributes and variables (KeyValue)
Copy link
Member

@Nfsaavedra Nfsaavedra left a comment

Choose a reason for hiding this comment

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

We are close :) I will open a new issue with the discussion about the handling of hashes in Puppet and Chef in order to let us proceed. The only thing I see missing now is this case.

$my_hash = {
	'key1' => {
    		'test1' => '1',
    		'test2' => '2',
    	},
	'key2' => 'value2',
	'key3' => 'value3',
}

$my_hash['key4'] = 'value4'

The last assignment should be parsed as the first one but with only key4 as another variable. I think this is the behavior for Chef, however in Puppet it is not working. Also I think Chef is merging the assignments in a single variable.

default[:zabbix][:database][:password] = nil
default[:zabbix][:test][:name] = "something"

This translates to the IR to only one default[:zabbix]. I am actually not sure how this works (and if it was you that programmed it), but it would be nice to have the same behavior in Puppet.

Let me know if anything does not make sense

@joaotgoncalves
Copy link
Collaborator Author

@Nfsaavedra , is the following example also possible? I would like to know your opinion about it.

$my_hash = {
	'key1' => {
    		'test1' => '1',
    		'test2' => '2',
    	},
	'key2' => 'value2',
	'key3' => 'value3',
}

$my_hash['key4']['key'5] = 'value4'

In this case, 'key4' has also an hash value right? So we should have '$my_hash' with keyvalues ['key1', 'key2', 'key3', 'key4'], 'key4' with keyvalues ['key5'] and 'key5' with value 'value4'?

Copy link
Member

@Nfsaavedra Nfsaavedra left a comment

Choose a reason for hiding this comment

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

LGTM. I will merge

@Nfsaavedra Nfsaavedra dismissed jff’s stale review March 9, 2023 18:18

Comments were already addressed

@Nfsaavedra Nfsaavedra merged commit a5fcf89 into main Mar 9, 2023
@Nfsaavedra Nfsaavedra deleted the ir_extension branch December 14, 2023 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants