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

Installing Local Operators Requires Abs or relative notation #1579

Merged
merged 4 commits into from
Jul 9, 2020

Conversation

kensipe
Copy link
Member

@kensipe kensipe commented Jul 7, 2020

Please read #1557 for full context. There has been confusion around specifying an operator name which is the same name as a folder in the CWD. The issue makes a lot of sense. There were a number of suggestions including sending details to STDOUT.

I'm not a fan of noisy CLIs... the information is there if you increase verbosity (-v=2).

The biggest issue is that if there is a folder with the same name as an operator, it is assumed to be what the user wants... all that was previously necessary is that fs.Stat(name) passes.. If this condition exists, there is no attempt to go to a repo etc.

The change makes necessary to specify a local operator with more than just a name... if it is in the CWD than install ./foo or install ./foo.tgz is necessary. It requires that more than the base name (go parlance) be supplied. It also accepts an absolute path. 2 things are now required... 1) it isn't just a filename and 2) the file or folder exists.

Signed-off-by: Ken Sipe kensipe@gmail.com

Fixes #1557

Signed-off-by: Ken Sipe <kensipe@gmail.com>
Signed-off-by: Ken Sipe <kensipe@gmail.com>
_, err = m.local.fs.Stat(name)
// force local operators usage to be either absolute or express a relative path
// or put another way, a name can NOT be mistaken to be the name of a local folder
if filepath.Base(name) != name && err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the comment to this method and the help for the cmd/install.go command (it's partially invalid now)

Copy link
Member Author

Choose a reason for hiding this comment

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

This comment is still valid. The human operator must provide an absolute path or express a rel path (and not just the folder name)

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant the fact that the relative path must begin with ./ e.g. ./kafka and not just kafka as was possible previously. In a way, this change might be even breaking for someone.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is not clear what you are asking for...
This could be considered a breaking change... but it is a breaking change for an operator dev... What is the likelihood that a regular user relies not on our repo but on a local file folder... and requiring a rel or abs path is an issue?
what we do have is a number of people and an issue that explicit states that the opposite is true... people are frustrated that an operator with the same name of an existing sub folder fails.

Signed-off-by: Ken Sipe <kensipe@gmail.com>
@kensipe kensipe added release/breaking-change This PR contains breaking changes and is marked in the release notes release/highlight This PR is a highlight for the next release labels Jul 9, 2020
Copy link
Contributor

@zen-dog zen-dog left a comment

Choose a reason for hiding this comment

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

🚢

Signed-off-by: Ken Sipe <kensipe@gmail.com>
@kensipe kensipe merged commit f58547c into main Jul 9, 2020
@kensipe kensipe deleted the ken/install-pathing branch July 9, 2020 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release/breaking-change This PR contains breaking changes and is marked in the release notes release/highlight This PR is a highlight for the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unclear messages when kubectl kudo install unexpectedly uses local directory
2 participants