Description

The CI jobs that install chef are failing due to the add-path deprecation as explained here: https://github.blog/changelog/2020-10-01-github-actions-deprecating-set-env-and-add-path-commands. This is blocking PRs from being evaluated/merged.

This PR is to set the ENV var that should allow the chef-install action in the ci jobs to complete.

Check List

[ ] All tests pass. See TESTING.md for details.
[ ] New functionality includes testing.
[ ] New functionality has been documented in the README if applicable.

JHBoricua commented 3 years ago

Hi, can someone please review this and push it through?

JHBoricua commented 3 years ago

Thank you Lance. We are back to running into the same deprecation issue with the windows cookbook that this cookbook depends on.

I saw that Tim Smith did a commit to address this (https://github.com/chef-boneyard/windows/commit/1f803a01d405de4f2a7e0b99042104e5744a22df) back in May 26th on that repo. Given that the cookbook is said to be deprecated and no longer maintained per the chef supermarket page for it, do you know if anyone is intending to update it in supermarket with the latest changes so that the ci jobs in this repo can complete?

Or are there any plans to break the dependency on the windows cookbook now that it is no longer maintained? Until this is resolved the only way to merge PRs in this repo would be by forcing the merge.

JHBoricua commented 3 years ago

Hi again Lance,

I think I resolved the windows cookbook dependency issue. The only thing I found using it was in the server.rb recipe, which was using the 'win_friendly_path' helper from the windows cookbook when creating the SQL installer config file. I can't think of a reason why this is needed anymore, as Chef can handle the windows paths natively without issue now. Maybe in the past that wasn't the case.

In any case, I removed the use of that windows_cookbook helper and removed the windows cookbook dependency from the metadata file. I also added the required line to the configure.rb & install.rb resource files to avoid the Chef Client 17+ deprecation warning issue. All the test kitchens on my end were successful after doing that. Please review at your convenience.

ramereth commented 3 years ago

@JHBoricua yeah removing that cookbook dep would be great. Looks like you need to fix the ChefSpec for this to move forward.

JHBoricua commented 3 years ago

@ramereth looks like 4 out of the 17 windows integration checks failed with what appears to be vagrant/virtualbox resource contention issues leading to either failure to launch the vm instance or timeout issues during the test. Not sure if the four failed checks can be re-run individually by you. Otherwise everything looks good.

JHBoricua commented 3 years ago

Good morning @ramereth,

I see the checks were kicked off again last night. This time 3 of the 17 integration tests failed instead of 4, so this seems to validate my suspicion that too many of the integration jobs are trying to run in parallel and are overwhelming the system that's running the VMs, leading to random timeouts. For this latest test cycle I see the following:

integration (windows-2016, install) - Chef successfully completed install and issued reboot. The VM either failed to reboot due to a locked process preventing it or failed to reboot fast enough for chef to reconnect in its allotted time, thus the job failed.
integration (windows-2016, server2016) - Chef timed out and was unable to connect to the VM via winrm 23 minutes after the install of SQL was initiated.
integration (windows-2019, install) - Chef successfully completed install and issued reboot. The VM either failed to reboot due to a locked process preventing it or failed to reboot fast enough for chef to reconnect in its allotted time, thus the job failed.

Are we ok with calling this good and allowing it to merge so that other PRs can be properly evaluated? I'm not sure what else I can do to alleviate the VM integration test issue, as I'm not too familiar with Github CI.

JHBoricua commented 3 years ago

Hi,

Quick ping on this PR. Can someone review the failed tests and my comments on them 9 days ago and make a call on whether to allow this to merge?

damacus commented 3 years ago

Thanks @JHBoricua, nice little fix!

Can you add an entry in the CHANGLOG under unreleased please?

JHBoricua commented 3 years ago

Hi @damacus, I added the entries you requested.

damacus commented 3 years ago

There's a bug with Windows 2016, but I think that's one for the next PR!

big thanks for fixing this!

kitchen-porter commented 3 years ago

Released as: 7.0.0

sous-chefs / sql_server

set env flag to allow chef install to complete in ci jobs #168

Description

Check List