-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Allow optional leading slash in CLI commands #2424
Conversation
7085175
to
02691a5
Compare
PG2 1762 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to test for triggers, rules, etc. Need to update the docs as well.
02691a5
to
4848b15
Compare
PG4 477 - Passed |
@mdeuser - in case you hadn't seen this, for when/if you have time for further review. |
// replaceMissingLeadSlash(name) returns a (possibly fully qualified) resource name, | ||
// inserting a leading '/' if it is of 3 parts (namespace/packageName/actionName) | ||
// and lacking the leading '/'. | ||
func replaceMissingLeadSlash(name string) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be more clear if this were a method on the QualifedName
struct? Perhaps rename it as well to a name that implies it only really works on a fully qualified entity name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it might be time for the QualifiedName
struct and associated methods, constructors, etc to reside in its own module.
if strings.HasPrefix(name, "/") { | ||
parts := strings.Split(name, "/") | ||
//parts = strings.Split(name, "/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove commented out code
@@ -112,7 +126,7 @@ func parseQualifiedName(name string) (QualifiedName, error) { | |||
return qualifiedName, err | |||
} | |||
|
|||
parts := strings.Split(name, "/") | |||
//parts = strings.Split(name, "/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove commented out code
@@ -95,6 +97,41 @@ class WskBasicTests | |||
stderr should include(errormsg) | |||
} | |||
|
|||
it should "accept a 3 part Fully Qualified Name without a leading '/'" in { | |||
val packageName = "packageName" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend using labels that can uniquely identify this particular test. If there's a need to traverse the backend logs, having unique labels is helpful
0d6b9aa
to
475de08
Compare
475de08
to
1f36484
Compare
PG4 511 Passed |
1f36484
to
4cefccf
Compare
@@ -0,0 +1,207 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the standard naming conventions for go files are to separate words using an underscore. Ie, quailifed_name.go.
|
||
// GetNamespaceFromQN() returns the name of the namespace in qualifiedName without | ||
// a leading '/' | ||
func (qualifiedName *QualifiedName) GetNamespaceFromQN() string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can drop FromQN
since it isn't in the other function names. Since the type is a QualifiedName
we can assume we can assume the from aspect.
try { | ||
validArgs foreach { | ||
cmd => | ||
val stderr = wsk.cli(cmd ++ wskprops.overrides ++ auth, expectedExitCode = SUCCESS_EXIT).stderr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cmd => wsk.cli(cmd ++ wskprops.overrides ++ auth, expectedExitCode = SUCCESS_EXIT)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need to check standard error here since we are expecting a SUCCESS_EXIT
code.
} | ||
} finally { | ||
cleanUp foreach { | ||
cmd => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop the new line after =>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@underwoodb-sd-ibm, please put the link to the issue in your PR description and prepend with "Closes ".
"../wski18n" | ||
) | ||
|
||
type QualifiedName struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that all the qualified name functions are now part of the QualifiedName type. Having all the functionality for a type consolidated in one place make the code easier to follow. 👍
4cefccf
to
740926f
Compare
qualifiedName.namespace = parts[1] | ||
|
||
if len(parts) < 2 || len(parts) > 4 { | ||
whisk.Debug(whisk.DbgError, "A valid qualified name was not detected\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is duplicated below. Can move the body of the if block to a function and just call that where needed.
@@ -34,8 +34,8 @@ func entityNameError(entityName string) (error) { | |||
return whisk.MakeWskError(errors.New(errMsg), whisk.EXITCODE_ERR_GENERAL, whisk.DISPLAY_MSG, whisk.DISPLAY_USAGE) | |||
} | |||
|
|||
func parseQualifiedNameError(entityName string, err error) (error) { | |||
whisk.Debug(whisk.DbgError, "parseQualifiedName(%s) failed: %s\n", entityName, err) | |||
func NewQualifiedNameError(entityName string, err error) (error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be moved into qualified_name.go?
740926f
to
050cc80
Compare
need to rebase. |
a972611
to
31b3cd0
Compare
PG2 1922 |
PG2 1922 - Passed |
d2b8312
to
11fb9f0
Compare
@underwoodb-sd-ibm can you move the tests out of the system package? sorry just noticed that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment
…thout leading slash '/' - Updated docs, fixed bug when creating Rule, improved and moved tests to WskBasicTests - Created new module for QualifiedName struct and functions * Built constructor and getter methods for QualifiedName, modified code to recognize QualifiedName pointers * Changed qualifiedname.go to qualified_name.go * Updated code to use getter functions for QualifiedName
11fb9f0
to
ff7e59a
Compare
@rabbah, tests have been moved to WskBasicUsageTests. Please confirm where I put it in the test is satisfactory, and if so, I'll tag Carlos for merging. Thanks! |
@underwoodb-sd-ibm, test is in the correct suite. |
@csantanapr - I'm assuming we need to wait for @rabbah to give the ok, but is it possible to merge this guy when/if you have time? Thanks! |
@rabbah if you are OK with the recent changes then adjust your review and merge away. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for moving the tests.
…thout leading slash in CLI (apache#2424)
…thout leading slash in CLI (apache#2424)
…thout leading slash in CLI (apache#2424)
…thout leading slash in CLI (apache#2424)
…thout leading slash in CLI (apache#2424)
…thout leading slash in CLI (apache#2424)
…thout leading slash in CLI (apache#2424)
…thout leading slash in CLI (apache#2424)
…thout leading slash in CLI (apache#2424)
…thout leading slash in CLI (apache#2424)
…thout leading slash in CLI (apache#2424)
This update is intended to allow the user to input a 3 part fully qualified name (FQN) with or without a leading '/' whenever entering a command.
Closes #2328