Skip to content
This repository has been archived by the owner on Mar 10, 2023. It is now read-only.

Add validation customer names list #393

Merged
merged 1 commit into from Feb 7, 2019

Conversation

viveksyngh
Copy link
Contributor

@viveksyngh viveksyngh commented Feb 6, 2019

Signed-off-by: Vivek Singh vivekkmr45@yahoo.in

Description

#391

How Has This Been Tested?

How are existing users impacted? What migration steps/scripts do we need?

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • read the CONTRIBUTION guide
  • signed-off my commits with git commit -s
  • added unit tests

Signed-off-by: Vivek Singh <vivekkmr45@yahoo.in>

if i != j {
if strings.HasPrefix(cn, customerName+"-") {
return false
Copy link
Contributor

Choose a reason for hiding this comment

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

i think it should also return a error msg specifying which username(s) caused this validation to fail and if possible, we should expose that in the PR automatically.

Copy link
Member

Choose a reason for hiding this comment

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

That's a good point. Perhaps we can change this to return the names which conflict @viveksyngh for debugging and better diagnostics?

@@ -11,3 +14,19 @@ func ValidateCustomers() bool {
}
return true
}

//ValidateCustomerList validate customer names list
Copy link
Contributor

Choose a reason for hiding this comment

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

where are we calling this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will be called inside function once we release new version of sdk and the vendor it inside the function.

for j, cn := range customers {

if i != j {
if strings.HasPrefix(cn, customerName+"-") {
Copy link
Contributor

Choose a reason for hiding this comment

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

while we are at it, do we want to add validation for duplicate check?

@alexellis
Copy link
Member

I'll merge this as the initial implementation.

A list of conflicting users would be ideal for a follow-up PR.

@alexellis alexellis merged commit de04098 into openfaas:master Feb 7, 2019
@alexellis
Copy link
Member

@martindekov @viveksyngh @s8sg please advise on which functions you believe we need to add this to?

I'm guessing system-github-event and system-gitlab-event?

@martindekov
Copy link
Contributor

Yes, for GitLab the system-gitlab-push has no functionality for validating customers(it was intended). I had issue somewhere to leave only system-*-event functions to have those(hmac validation,customers ...) and depricate those from the system-*-push functions.

@martindekov
Copy link
Contributor

But isn't the CUSTOMER validation already added ?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants