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

Hide the registry flag [-R] behind experimental #608

Closed
yaelharel opened this issue May 6, 2020 · 5 comments · Fixed by #619
Closed

Hide the registry flag [-R] behind experimental #608

yaelharel opened this issue May 6, 2020 · 5 comments · Fixed by #619
Assignees
Labels
status/in-progress Issue or PR that is currently in progress. type/enhancement Issue that requests a new feature or improvement.
Milestone

Comments

@yaelharel
Copy link
Contributor

Description

The registry is still an early beta. We would like to ensure that users are aware of that. Therefore, we would like that the registry flag -R will be visible only if the experimental flag is on in the config.

Proposed solution

# Experimental flag is false or not set
Given experimental is false or not set
When I run `pack create-builder MY_BUILDER -R REGISTRY`
Then I should see the following message:
ERROR: Support for using buildpackages in a CNB Registry is currently experimental.
And exit code should be non-zero

# Experimental flag is true
Given experimental is true
When I run `pack create-builder MY_BUILDER -R REGISTRY`
Then creating the builder should succeed
And exit code should be zero

Additional context

The experimental flag was introduced in this PR.
The registry flag was introduced in this PR.

The feature should be documented as part of this issue.

@yaelharel yaelharel added status/triage Issue or PR that requires contributor attention. type/enhancement Issue that requests a new feature or improvement. labels May 6, 2020
@jromero jromero added status/ready Issue ready to be worked on. and removed status/triage Issue or PR that requires contributor attention. labels May 6, 2020
@jromero jromero added this to the 0.11.0 milestone May 6, 2020
@natalieparellano natalieparellano added status/in-progress Issue or PR that is currently in progress. and removed status/ready Issue ready to be worked on. labels May 11, 2020
@jromero jromero linked a pull request May 12, 2020 that will close this issue
@yaelharel
Copy link
Contributor Author

Following a discussion with @sclevine, we consider making two changes to the error message:

  1. Write "buildpacks" instead of "buildpackages".
  2. Indicate that the "experimental" flag can be set in the config file.

So the updated error message will be:
"ERROR: Support for using buildpacks in a CNB Registry is currently experimental. If you would like to try it, add experimental = true to your config file."

@jromero, can you please share your opinion?

Thanks!

@jromero
Copy link
Member

jromero commented May 13, 2020

Minor feedback:

  1. Given that the flag is --buildpack-registry, the error message should also replace "CNB Registry" with "buildpack registry".
  2. We have a Tip format to provide "actions" the user can take in a consistent manner.
  3. Bonus, we should provide the path to the config file for ease of use.

With that in mind, what do you think about this output?

ERROR: Support for buildpack registries is currently experimental.

Tip: To enable experimental features, add `experimental = true` to {{CONFIG_PATH}}.

As far as implementation, this should be achievable by creating a custom error type that can be returned anywhere an experimental feature is to be gated. Then the error type would only need to be checked against in the cmd package. When this error is received, we would then print the tip after printing the error. This would help prevent a lot of duplication and cleanly split the responsibilities between packages. Let me know if my explanation isn't clear.

@yaelharel
Copy link
Contributor Author

@jromero, thank you for your feedback! Your idea sounds good.
I'll make these changes.

@yaelharel yaelharel self-assigned this May 13, 2020
@yaelharel
Copy link
Contributor Author

yaelharel commented May 13, 2020

@jromero After giving it another thought, what do you think about the following solution:

  • Create a new file experiment_error.go in the commands package. The file will contain an ExperimentError struct with a string (the message) and another constant string (the tip string).
  • The Error() function of the struct will return a concatenation of the message and the tip.

In this way, we won't need to make any changes in the cmd package and as far as I know, the ExperimentError relates only to the commands package.

Would love to get your opinion.

@jromero
Copy link
Member

jromero commented May 13, 2020

First point makes sense but the second might not work in actuality since you'll want to use the logger to take advantage of it's formatting functions for things such as colors. You could pass the logger to the Error type but I'd be wary of adding logic to a data type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/in-progress Issue or PR that is currently in progress. type/enhancement Issue that requests a new feature or improvement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants