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

proposed implementation of distro constants #112

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bobchaos
Copy link

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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (non-breaking change that does not add functionality or fix an issue)

Checklist:

  • I have read the CONTRIBUTING document.
  • I have run the pre-merge tests locally and they pass.
  • [NA] I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commits have been signed-off for the Developer Certificate of Origin.

@bobchaos bobchaos requested review from a team as code owners November 12, 2019 18:37
Copy link
Contributor

@tyler-ball tyler-ball left a 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

WEBSITE = "https://chef.io".freeze

# chef-apply's product name
APPLY = "Chef Infra Apply".freeze
Copy link
Contributor

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?

Copy link

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. 🥇

Copy link
Member

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.

Copy link
Author

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.

Copy link
Member

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?

Copy link
Author

@bobchaos bobchaos Feb 6, 2020

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

Copy link
Member

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.

@@ -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))
Copy link
Contributor

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
Copy link

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. 😄

Copy link

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 👍

Copy link
Author

@bobchaos bobchaos Nov 13, 2019

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

@bobchaos
Copy link
Author

I'll give this thing a quick rebase. Thanks @afiune !

@afiune afiune added Aspect: Packaging Distribution of the projects 'compiled' artifacts. Status: Adopted An issue that is being worked on. Triage: Confirmed Indicates and issue has been confirmed as described. labels Feb 12, 2020
@bobchaos
Copy link
Author

Just going over my pending PRs in preparation of May 1st (next week! :O ). Anything missing to get this merged?

@ramereth
Copy link
Contributor

ramereth commented Jun 9, 2020

@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!

Copy link
Member

@marcparadise marcparadise left a 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.

bobchaos and others added 2 commits June 10, 2020 08:58
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Aspect: Packaging Distribution of the projects 'compiled' artifacts. Status: Adopted An issue that is being worked on. Triage: Confirmed Indicates and issue has been confirmed as described.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants