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

Added changes to support WWPNs in host. #176

Merged
merged 8 commits into from
Mar 13, 2024
Merged

Conversation

chitranm
Copy link
Contributor

Added WWPNs field in host read.

@chitranm chitranm requested a review from a team as a code owner March 10, 2024 06:25
internal/resources/resource_host.go Outdated Show resolved Hide resolved
internal/resources/resource_host.go Outdated Show resolved Hide resolved
Comment on lines 600 to 602
d.Set(hCHAPUser, host.ISCSIConfig.CHAPUser)
d.Set(hCHAPSecret, host.ISCSIConfig.CHAPSecret)
d.Set(hInitiatorName, host.ISCSIConfig.InitiatorName)
Copy link

@shi98382 shi98382 Mar 12, 2024

Choose a reason for hiding this comment

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

Looks like iscsi information will always be filled in host here regardless protocol. Is it correct that the protocol field was introduced to differentiate the cases, otherwise it will defeat the purpose. Just FYI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @shi98382, please check this comment from Mike - https://github.com/HewlettPackard/hpegl-metal-terraform-resources/pull/176/files/b7e0b747020293dd264e681bea394e6506632e6d#r1520373211

I have reverted back the protocol check after this discussion.

@chitranm
Copy link
Contributor Author

Hi @ron-saito, if you are ok with my response to your suggestion then please let me know. I will merge this PR.

@chitranm
Copy link
Contributor Author

@mchuang3, @shi98382 and @ron-saito I have accepted Ron's changes and CI build is going on. Once that is done I will update this branch with master and will merge it.

@@ -590,6 +600,13 @@ func resourceMetalHostRead(d *schema.ResourceData, meta interface{}) (err error)
d.Set(hCHAPUser, host.ISCSIConfig.CHAPUser)
d.Set(hCHAPSecret, host.ISCSIConfig.CHAPSecret)
d.Set(hInitiatorName, host.ISCSIConfig.InitiatorName)

if len(host.WWPNs) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

While this was previously discussed, I'm wondering why this is needed (conditional set). without this check, hWWPNs is set to whatever host.WWPNs is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just accepted your suggestion on the PR as is. I was also thinking why this is needed. If you are ok I will remove this check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed your comment.

@chitranm chitranm merged commit e25575f into master Mar 13, 2024
2 checks passed
@chitranm chitranm deleted the US60196_host_wwpns_support branch March 13, 2024 17:58
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