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

ensure ingress-nginx node label is applied #377

Merged
merged 2 commits into from
Aug 29, 2024

Conversation

nabuskey
Copy link
Collaborator

fixes: #366
fixes: #367

Copy link
Contributor

@nimakaviani nimakaviani left a comment

Choose a reason for hiding this comment

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

just a minor typo and a comment

pkg/kind/cluster.go Outdated Show resolved Hide resolved
Comment on lines 258 to 279

nodes:
for i := range parsedCluster.Nodes {
node := parsedCluster.Nodes[i]
for _, pm := range node.ExtraPortMappings {
if strconv.Itoa(int(pm.HostPort)) == c.cfg.Port {
appendNecessaryPort = false
nodePosition = i
if node.Labels != nil {
v, ok := node.Labels[ingressNginxNodeLabelKey]
if ok && v == ingressNginxNodeLabelValue {
appendIngresNodeLabel = false
}
}
break nodes
}
}
if node.Labels != nil {
v, ok := node.Labels[ingressNginxNodeLabelKey]
if ok && v == ingressNginxNodeLabelValue {
appendIngresNodeLabel = false
nodePosition = i
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this can be further simplified. I am not fond of label breaks in go, but leave it up to you.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's really just comes down to last one winning or the first one winning. I chose first one winning. If we do last one wins, we can avoid using label breaks.

Copy link
Contributor

Choose a reason for hiding this comment

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

i like first one winning too.

Here is an alternative way of writing it (thanks ChatGPT). I dont think it is any easier tho. let's leave it.

for i, node := range parsedCluster.Nodes {
    checkNodeLabels := func() {
        if node.Labels != nil {
            if v, ok := node.Labels[ingressNginxNodeLabelKey]; ok && v == ingressNginxNodeLabelValue {
                appendIngresNodeLabel = false
            }
        }
    }

    for _, pm := range node.ExtraPortMappings {
        if strconv.Itoa(int(pm.HostPort)) == c.cfg.Port {
            appendNecessaryPort = false
            nodePosition = i
            checkNodeLabels()
            break
        }
    }

    if appendNecessaryPort { // If the port was not matched, check labels
        checkNodeLabels()
        if !appendIngresNodeLabel {
            nodePosition = i
            break
        }
    }
}

Signed-off-by: Manabu McCloskey <[email protected]>
@nimakaviani nimakaviani self-requested a review August 29, 2024 22:14
Copy link
Contributor

@nimakaviani nimakaviani left a comment

Choose a reason for hiding this comment

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

LGTM

@nimakaviani nimakaviani merged commit caf882e into cnoe-io:main Aug 29, 2024
5 checks passed
tapas4java pushed a commit to tapas4java/idpbuilder that referenced this pull request Sep 3, 2024
Signed-off-by: Manabu McCloskey <[email protected]>
Signed-off-by: Tapas Jena <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants