-
Notifications
You must be signed in to change notification settings - Fork 14
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
proposed implementation of distro constants #112
base: main
Are you sure you want to change the base?
Conversation
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.
1 question but generally I think this looks good and starightforward. Thanks @bobchaos
lib/chef_apply/dist.rb
Outdated
WEBSITE = "https://chef.io".freeze | ||
|
||
# chef-apply's product name | ||
APPLY = "Chef Infra Apply".freeze |
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.
Hmm, hadn't thought about this one. Seems right to me, but I could be persuaded that Chef Apply
is better. I think Chef Infra Apply
is more appropriate since it is a subset of the chef
command. @chef/chef-workstation-owners what do y'all think?
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 would personally check with marketing/docs teams. I vote as is Chef Infra Apply
. 🥇
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've sent the question over to marketing and will post back with any updates.
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.
Any news on this? I'm going through workstation dependencies to update Cinc's backlog.
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 turns out to be a little tricky, because 'chef-apply' (chef-run) isn't a product itself. We're continuing to have discussions around this internally, but I don't yet have a time frame for when we'll have a definitive answer.
It looks like we're not referencing this in the chef_apply repository - would it make sense to remove this const for now, or would that cause problems for Cinc's implementation?
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.
If it's not a word we need to worry about then it could certainly be removed. I'll try to book some time to fix up this PR over the next two weeks, we can at least see how it looks. Only worry I can think of is it's going to look a bit weird to have a single tool in workstation still be called chef-*
where everything else is cinc-*
, but we can address that by patching + wrappers until a definitive answer is found
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 we go with "chef-run"? It'll eliminate confusion around chef-apply/chef-run and side-steps the whole "isn't a product" problem.
lib/chef_apply/startup.rb
Outdated
@@ -71,9 +75,9 @@ def run | |||
rescue ConfigPathInvalid => e | |||
UI::Terminal.output(T.error.bad_config_file(e.path)) | |||
rescue ConfigPathNotProvided | |||
UI::Terminal.output(T.error.missing_config_path) | |||
UI::Terminal.output(T.error.missing_config_path(D::RUNEXEC, D::DK)) |
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 is definitely appropriate looking at the error message, but internally we have a ticket to change this from .chefdk
to .chef-workstation
. We'll come back and clean this up 😄
|
||
module ChefApply | ||
module Action | ||
class InstallChef < Base | ||
class MinimumChefVersion | ||
|
||
D = ChefApply::Dist |
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 feel this is a bit obscure but, I also don't like typing that much so, it is fine I guess. 😄
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.
Also, other parts of the codebase do it, so 👍
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.
all the ChefApply::Dist::SOMEREALLYLONGSTRINGS
were making my eyes bleed o.O
I'll give this thing a quick rebase. Thanks @afiune ! |
Just going over my pending PRs in preparation of May 1st (next week! :O ). Anything missing to get this merged? |
@marcparadise @tyler-ball I rebased this the other day. I was hoping you could take another look at this so we can start using this. Thanks! |
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.
Just one minor change, setting "Chef Infra Apply" to "chef-run". We'll just call it what the command name is for now, so that the open product question doesn't block this any longer.
Also, please run rake style:autocorrect
to clear style warning so that the lint tests pass.
If the Windows failure continues on next build, I'll take a look and see what we need to fix in deps.
Signed-off-by: Marc Chamberland <chamberland.marc@gmail.com>
Signed-off-by: Lance Albertson <lance@osuosl.org>
|
||
Download the latest here: | ||
|
||
https://downloads.chef.io/chef-workstation/stable | ||
https://%4/chef-workstation/stable | ||
|
||
version: | ||
description: Show the current version of Chef Run. |
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.
description: Show the current version of Chef Run. | |
description: Show the current version of %1. |
|
||
Download the latest here: | ||
|
||
https://downloads.chef.io/chef-workstation/stable | ||
https://%4/chef-workstation/stable |
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.
https://%4/chef-workstation/stable | |
https://%4/%5/stable |
rescue UnsupportedInstallation | ||
UI::Terminal.output(T.error.unsupported_installation) | ||
UI::Terminal.output(T.error.unsupported_installation(ChefApply::Dist::RUNEXEC, ChefApply::Dist::DK, ChefApply::Dist::WORKSTATION, ChefApply::Dist::DOWNLOADS_URL)) |
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.
UI::Terminal.output(T.error.unsupported_installation(ChefApply::Dist::RUNEXEC, ChefApply::Dist::DK, ChefApply::Dist::WORKSTATION, ChefApply::Dist::DOWNLOADS_URL)) | |
UI::Terminal.output(T.error.unsupported_installation(ChefApply::Dist::RUNEXEC, ChefApply::Dist::DK, ChefApply::Dist::WORKSTATION, ChefApply::Dist::DOWNLOADS_URL, ChefApply::Dist:: WORKSTATION_URL_SUFFIX)) |
Signed-off-by: Marc Chamberland chamberland.marc@gmail.com
Description
In order to support redistributing workstation we need to remove Chef Inc trademarks from chef-apply/chef-run. This PR proposes an implementation based on what is currently accepted for chef/chef as described here: chef/chef#8376
Related Issue
None, but I can open one if needed, either here or VS the workstation repo.
Types of changes
Checklist: