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

Add node_pools to outputs #401

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

SubatomicHero
Copy link

Describe your changes

This change adds the node_pools created with var.node_pools to the outputs. Useful for local testing of the module

Checklist before requesting a review

  • The pr title can be used to describe what this pr did in CHANGELOG.md file
  • I have executed pre-commit on my machine
  • I have passed pr-check on my machine

@zioproto
Copy link
Collaborator

Hello @SubatomicHero ,
could you please make an example of why we need this PR ?
can you do a practical example of something you are testing that requires this output ?
thanks

Copy link
Member

@lonegunmanb lonegunmanb left a comment

Choose a reason for hiding this comment

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

Thanks @SubatomicHero for opening this pr! We don't encourage outputting resource directly in the module, I've checked data.azurerm_kubernetes_cluster_node_pool and all it requires is cluster name, pool name and resource group name.

Could we only output the pools' names like this?:

output "pool_name" {
  value = { for k, p in azurerm_kubernetes_cluster_node_pool.node_pool : k => p.name }
}

@fleetwoodstack
Copy link

@lonegunmanb sorry for the late reply. That's been done and I agree with your change! This is now ready to review again

@zioproto the use case is so that we can run our terraform test assertions making sure the extra node pool is configured a certain way. We need access to the azurerm_kubernetes_cluster_node_pool.node_pool output to allow this.

@fleetwoodstack
Copy link

@zioproto is there anything else this PR is waiting on to progress?

I've taken over since the PR owner has since left our organisation so won't be contributing to it.

@zioproto
Copy link
Collaborator

@fleetwoodstack LGTM. I don't have ownership over this repo to merge the PR. Friendly ping to @lonegunmanb to approve the CI workflow and eventually merge :) Thanks

@fleetwoodstack
Copy link

@lonegunmanb are you OK to mark your requested changes as having been done? I don't own this PR sadly to be able to mark them as completed.

@fleetwoodstack
Copy link

@lonegunmanb When you get a moment, this is ready to merge

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