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.

Sign up for GitHub

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

Jump to bottom

Package new chef-analyze binary #553

Merged
merged 4 commits into from
Oct 2, 2019
Merged

Package new chef-analyze binary #553

merged 4 commits into from
Oct 2, 2019

Conversation

afiune
Copy link

@afiune afiune commented Oct 1, 2019 •
edited
Loading

Description

Package the new chef-analyze Go binary. (https://github.com/chef/chef-analyze)

Signed-off-by: Salim Afiune afiune@chef.io

Related Issue

GH Issue: #552
Depends on chef/omnibus-software#1098

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

@afiune afiune added Aspect: Packaging Distribution of the projects 'compiled' artifacts. Aspect: Performance Works without negatively affecting the system running it. Status: Adopted An issue that is being worked on. labels Oct 1, 2019
@afiune afiune requested review from a team as code owners October 1, 2019 06:37
marcparadise
marcparadise reviewed Oct 1, 2019
View reviewed changes
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.

Let's get this through a pipeline build now that the changes are all in, then we're good to merge.

@afiune afiune mentioned this pull request Oct 1, 2019
Merged
@afiune
Copy link
Author

afiune commented Oct 1, 2019

Adhoc https://buildkite.com/chef/chef-chef-workstation-master-omnibus-adhoc/builds/105 running!

tas50
tas50 reviewed Oct 1, 2019
View reviewed changes
omnibus/config/projects/chef-workstation.rb
@@ -99,6 +99,11 @@
dependency "uninstall-scripts"
dependency "ruby-cleanup"

dependency "go"
Copy link
Contributor

Choose a reason for hiding this comment

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

you shouldn't need the go dependency here if the analyze project config also deps on it

Copy link
Member

Choose a reason for hiding this comment

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

We discussed this and wanted to make it explicit because there's no clear indication of what's installing the go binary, or even if it's installed, at the time we add the cleanup dependency further down. It was more for humans than for omnibus.

Copy link
Author

Choose a reason for hiding this comment

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

What Marc said ☝️ -- Though, I would like to ask @tyler-ball for his point of view. This is just a matter of clarity for the team since now we will be shipping Ruby, Rust and Go binaries.

@afiune afiune force-pushed the afiune/552/software-definition/chef-analyze branch from 15fa4e3 to d0b6fbc Compare October 1, 2019 19:25
afiune pushed a commit to chef/chef-analyze that referenced this pull request Oct 1, 2019
Adds chef-analyze help / Usage()
09bd26e 
The usage here is to enable our pipeline to check if the chef-analyze
command was built and install and runs. (Minimal check)

chef/chef-workstation#553

Signed-off-by: Salim Afiune <afiune@chef.io>
@afiune afiune mentioned this pull request Oct 1, 2019
Merged
@afiune
Copy link
Author

afiune commented Oct 1, 2019 •
edited
Loading

@marcparadise The adhoc build passed! Though we made a mistake with the file_extension logic

tenor-62851523

➜  chef-workstation git:(afiune/552/software-definition/chef-analyze) /opt/chef-workstation/bin/chef-analyze.exe foo
One or more parameters missing.

Required:
	-user USER
	-key KEY
	-chef_server_url URL

I have fixed it and will add a test to verify we are shipping it correctly. 💯

@afiune
Copy link
Author

afiune commented Oct 1, 2019

Another Build with the right extension: https://buildkite.com/chef/chef-chef-workstation-master-omnibus-adhoc/builds/106#f093266c-5971-4335-bc8b-504b0cd4390d

tenor-59587725

@afiune afiune self-assigned this Oct 1, 2019
@afiune afiune mentioned this pull request Oct 1, 2019
Merged
Salim Afiune added 4 commits October 2, 2019 12:31
@marcparadise
Package new chef-analyze binary
0560afe 
Depends on chef/omnibus-software#1098

Signed-off-by: Salim Afiune <afiune@chef.io>
@marcparadise
Be as explicit as possible
89d6e2d 
For the chef-analyze software definition, have the command to be similar
for both, Unix and Windows.

When adding the dependency to the Chef Workstation project, be also
explicit to depend on go and then uninstall it.

Signed-off-by: Salim Afiune <afiune@chef.io>
@marcparadise
Correct file extention
92175e9 
Signed-off-by: Salim Afiune <afiune@chef.io>
@marcparadise
Adds basic chef-analyze verification
ab26b12 
Signed-off-by: Salim Afiune <afiune@chef.io>
@marcparadise marcparadise force-pushed the afiune/552/software-definition/chef-analyze branch from 3b45cc7 to ab26b12 Compare October 2, 2019 16:32
@marcparadise marcparadise merged commit a9dbe0d into master Oct 2, 2019
@chef-expeditor chef-expeditor bot deleted the afiune/552/software-definition/chef-analyze branch October 2, 2019 16:46
@afiune afiune mentioned this pull request Oct 7, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tas50 tas50 tas50 left review comments

@marcparadise marcparadise marcparadise approved these changes

Assignees

@afiune afiune

Labels
Aspect: Packaging Distribution of the projects 'compiled' artifacts. Aspect: Performance Works without negatively affecting the system running it. Status: Adopted An issue that is being worked on.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@afiune @tas50 @marcparadise