This repository has been archived by the owner on Nov 15, 2022. It is now read-only.
Added case for lowercase protocol #564
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pradeepto
requested review from
kadel,
concaf,
surajssd,
cdrage and
surajnarwade
|
Thank you @piyush1594 for fixing issues in the Open Source spirit. |
kadel
reviewed
Jan 8, 2018
pkg/spec/resources.go
Outdated
| @@ -669,9 +669,10 @@ func parsePortMapping(pm string) (*api_v1.ServicePort, error) { | |||
| // Case 3 - port/protocol | |||
| // Case 4 - port:targetPort/protocol | |||
| case 2: | |||
| switch api_v1.Protocol(protocolSplit[1]) { | |||
| protocol_uppercase := strings.ToUpper(protocolSplit[1]) | |||
There was a problem hiding this comment.
just one small nitpick 😇
protocolUpperCase
Go formatting style prefers camelCase names.
Yes, I know we have other variables with an underscore like api_v1 😄 , but that is a special case.
Normal variables should be camelCase
|
Code LGTM. Same as @kadel just change to camelCase and let's merge 👍 |
cdrage
approved these changes
Jan 8, 2018
surajnarwade
approved these changes
Jan 8, 2018
|
LGTM (after camelCase) |
This will accept if someone specifies protocol as tcp or Tcp or tCp. Likewise UDP also Always return the protocol as Uppercase Added test for two cases changed variable name to CamelCase
piyush-garg
force-pushed
the
lowercase_protocol
branch
from
a954f7b to
ced8e66
Compare
|
Changes requested by you are done. Please take a look. Thanks, @pradeepto @kadel @cdrage @surajnarwade |
|
LGTM! Thank you 👍 |
kadel
pushed a commit
to kadel/kedge
that referenced
this pull request
Jan 15, 2018
This will accept if someone specifies protocol as tcp or Tcp or tCp. Likewise UDP also Always return the protocol as Uppercase Added test for two cases changed variable name to CamelCase
kadel
pushed a commit
to kadel/kedge
that referenced
this pull request
Jan 17, 2018
This will accept if someone specifies protocol as tcp or Tcp or tCp. Likewise UDP also Always return the protocol as Uppercase Added test for two cases changed variable name to CamelCase
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This will accept if someone specifies protocol as
tcp or Tcp or tCp. Likewise UDP also
Always return the protocol as Uppercase
Added test for two cases
#560