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

Switch sensu_ctl resouce to use archive_file #112

Merged
merged 5 commits into from
Oct 28, 2020
Merged

Switch sensu_ctl resouce to use archive_file #112

merged 5 commits into from
Oct 28, 2020

Conversation

webframp
Copy link
Contributor

@webframp webframp commented Oct 17, 2020

This removes the need for a dependency on the seven_zip cookbook since
archive_file is a builtin chef resources since Chef 15

fixes #110


Pull Request Checklist

Is this in reference to an existing issue?

Yes.

General

  • Update Changelog following the conventions laid out here

  • Update README with any necessary configuration snippets

  • Cookstyle (rubocop) passes

  • Rspec (unit tests) passes

  • Inspec (integration tests) passes

New Features

  • Added a Testing Artifact as either an automated test or a manual artifact on the PR.

  • Added documentation for it to the README.md

Purpose

Modernize archive handling and remove dependency on seven_zip cookbook

Known Compatibility Issues

Unknown. Windows testing is not complete in CI, will need manual testing.

@webframp webframp requested a review from a team as a code owner October 17, 2020 20:04
@webframp
Copy link
Contributor Author

webframp commented Oct 17, 2020

@derekgroh Can you take a look at this and confirm that behavior is the same as before? I don't easily have a Windows setup to do enough testing on. Once we know this works the same I can update the readme docs and changelog.

@webframp webframp changed the title Switch sensu_ctl resouce to use archive_file Draft: Switch sensu_ctl resouce to use archive_file Oct 17, 2020
@webframp webframp changed the title Draft: Switch sensu_ctl resouce to use archive_file Switch sensu_ctl resouce to use archive_file Oct 17, 2020
Copy link
Contributor

@majormoses majormoses left a comment

Choose a reason for hiding this comment

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

LGTM but needs testing on windows which I can't really provide

@webframp webframp marked this pull request as draft October 17, 2020 20:27
@webframp
Copy link
Contributor Author

@majormoses same for me, marked as Draft for now until we can confirm behavior better on Windows

Copy link
Contributor

@derekgroh derekgroh left a comment

Choose a reason for hiding this comment

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

Nevermind, my branch doesn't have the Chef 15+ pin.

Will get this validated and let you know.

@derekgroh
Copy link
Contributor

enterprise needs to be removed from line 70 in the -Outfile

Had some networking issues that I had to work around related to the configuration action, but the install action is working successfully with the change.

We may want to introduce some guards to help speed up the run after it is setup as it runs through each resource every time.. Unfortunately, the exe doesn't publish with any version info to use so we'd have to track with another method, maybe a version.info file.

@webframp
Copy link
Contributor Author

Good feedback @derekgroh thanks! I'll be working on the cookbook again over this weekend as I have time so any suggestions are appreciated

@webframp
Copy link
Contributor Author

Rebased on latest master

Copy link
Contributor

@majormoses majormoses left a comment

Choose a reason for hiding this comment

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

Lets only extract when we download a version rather than every chef run.

resources/ctl.rb Outdated Show resolved Hide resolved
@derekgroh
Copy link
Contributor

derekgroh commented Oct 27, 2020

This is now failing on windows with #116 merged in.

Resource needs to be updated to:

default_ca_cert = value_for_platform_family(
  %w(rhel fedora amazon) => '/etc/ssl/certs/ca-bundle.crt',
  'debian' => '/etc/ssl/certs/ca-certificates.crt',
  'windows' => ''
)

There seems to be an odd bug with the :configure resource as well as sensuctl isn't available to the shell until the second converge with execute, but powershell_script is successful. Not sure if this is an environment specific issue (found on 2012R2, 2016 and 2019) as I recall it working in the past without issue.

@webframp
Copy link
Contributor Author

There seems to be an odd bug with the :configure resource as well as sensuctl isn't available to the shell until the second converge with execute, but powershell_script is successful. Not sure if this is an environment specific issue (found on 2012R2, 2016 and 2019) as I recall it working in the past without issue.

You might be referring to the behavior I saw a while back that led to my attempt at a fix in #74

I'm not sure of the state of change now since it's a bit old. It should be reviewed though to ensure correct behavior.

@webframp webframp marked this pull request as ready for review October 28, 2020 02:35
This removes the need for a dependency on the seven_zip cookbook since
archive_file is a builtin chef resources since Chef 15
Using the `archive_file` resource means less resources are needed and
have been removed. Also removing corresponding spec tests here.
We should extract only when we download new files, not everytime.

from review: @majormoses
@webframp webframp merged commit 3f7ca2a into master Oct 28, 2020
@webframp webframp deleted the archive-file branch October 28, 2020 02:46
@derekgroh
Copy link
Contributor

@webframp It is a Windows Path issue being immediately available to the execute resource.

My testing showed this could be solved by using powershell_script resource instead.

The challenge is you have to ensure that the test runs successfully the first time, because any secondary run will succeed, but only because the path value is already set.

derekgroh pushed a commit to derekgroh/sensu-go-chef that referenced this pull request Nov 19, 2020
* Switch sensu_ctl resouce to use archive_file

This removes the need for a dependency on the seven_zip cookbook since
archive_file is a builtin chef resources since Chef 15

* Remove unneeded specs for archive_file

Using the `archive_file` resource means less resources are needed and
have been removed. Also removing corresponding spec tests here.

* Fix indentation on ctl resource

* Switch to notify for archive_file action

We should extract only when we download new files, not everytime.

from review: @majormoses

* Update CHANGELOG for sensu_ctl resource changes

also removed duplicate line
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use archive_file resource instead of seven_zip_archive
3 participants