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 #YUPTOO-37 - omit nics starts with & associated mac_addrs #29

Merged

Conversation

kgaikwad
Copy link
Collaborator

@kgaikwad kgaikwad commented Sep 6, 2022

No description provided.

@patilsuraj767
Copy link
Collaborator

Hello @kgaikwad Thanks for creating the PR, Modifiers are designed to be independent of each other. One modifier should not import the other. Will request you to shift whole code under single modifier.

@kgaikwad
Copy link
Collaborator Author

kgaikwad commented Sep 6, 2022

Hello @kgaikwad Thanks for creating the PR, Modifiers are designed to be independent of each other. One modifier should not import the other. Will request you to shift whole code under single modifier.

As I am modifying mac_addresses field details of host, I won't prefer to move mac_addresses specific code into file named as transform_network_interfaces.py. In addition to this, logic extracted into a static method so it can be easily imported from anywhere.
My intention was to keep all transformations related to mac_addresses inside one file i.e transform_mac_addresses.py so that in future one can add changes just by looking into single file and anticipate the impact of change.
I can move this method to different utils file, will it work for you? thoughts?

@patilsuraj767
Copy link
Collaborator

Yes, we can move the logic in different utils file which is outside modifier directory. Once issue 30 is fixed we can move the code to new modifier file.

@kgaikwad
Copy link
Collaborator Author

kgaikwad commented Sep 8, 2022

Moved helper method to different file. Please request to review.

@kgaikwad
Copy link
Collaborator Author

kgaikwad commented Sep 8, 2022

Once issue 30 is fixed we can move the code to new modifier file.

Btw, I didn't get how this is relevant to issue 30. As per understanding these are two different things.

Copy link
Contributor

@r14chandra r14chandra left a comment

Choose a reason for hiding this comment

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

lgtm! 👍🏻

@patilsuraj767
Copy link
Collaborator

Once issue 30 is fixed we can move the code to new modifier file.

Btw, I didn't get how this is relevant to issue 30. As per understanding these are two different things.

@kgaikwad what I meant was once we have issue 30 fixed and once we have dependency graph mechanism in place. Then we can create a separate modifier for omitting nics and add its dependency in transform_network_interfaces.py. That way all the code can be in one file + transform_network_interfaces modifier will run after omitting nics modifier.

Copy link
Collaborator

@patilsuraj767 patilsuraj767 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @kgaikwad, Approving and merging it.

@patilsuraj767 patilsuraj767 merged commit 488fe57 into RedHatInsights:main Sep 8, 2022
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.

3 participants