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

Support JWT auth in mergeable minions #311

Merged
merged 1 commit into from
Aug 7, 2018
Merged

Conversation

pleshakov
Copy link
Contributor

@pleshakov pleshakov commented Jul 13, 2018

Copy link
Contributor

@Dean-Coakley Dean-Coakley left a comment

Choose a reason for hiding this comment

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

Looks good. Few small suggestions

}
}
}
}

func (lbc *LoadBalancerController) findIngressesForSecret(secret string) ([]extensions.Ingress, error) {
res := []extensions.Ingress{}
func (lbc *LoadBalancerController) findIngressesForSecret(secret string) ([]extensions.Ingress, []extensions.Ingress, error) {
Copy link
Contributor

@Dean-Coakley Dean-Coakley Jul 13, 2018

Choose a reason for hiding this comment

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

I think we should use named return types so that nonMinions and minions are not mixed up in future extensions of this method

}

// we're dealing with a minion
// only JWT secrets are allowed in a minion
Copy link
Contributor

Choose a reason for hiding this comment

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

only JWT secrets are allowed in a minion->JWT secrets are only allowed in a minion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, JWT secrets are allowed in both minions and non-minions
however, minions can only have JWT secrets.

@isserrano
Copy link
Contributor

Ignore the jenkins check for this one

@pleshakov pleshakov force-pushed the mergeable-ingresses-jwt branch 2 times, most recently from b99831a to 1fd1127 Compare August 1, 2018 15:41
Copy link
Contributor

@isaachawley isaachawley left a comment

Choose a reason for hiding this comment

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

👍

@pleshakov pleshakov added the enhancement Pull requests for new features/feature enhancements label Aug 3, 2018
@pleshakov pleshakov merged commit 0c85428 into master Aug 7, 2018
@pleshakov pleshakov deleted the mergeable-ingresses-jwt branch August 7, 2018 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Pull requests for new features/feature enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants