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

Only accept MM/DD/YYYY for windows_task start_day #6434

Merged
merged 5 commits into from
Sep 26, 2017
Merged

Conversation

jaym
Copy link
Contributor

@jaym jaym commented Sep 22, 2017

DateTime.parse is not really that well defined. It accepts a lot
of different dates, however not MM/DD/YYYY. It does DD/MM/YYYY.
We should be very specific in the format we accept, and continue
to support MM/DD/YYYY as it was in the cookbook

Fixes chef-boneyard/chef-client#523

@jaym jaym requested a review from a team September 22, 2017 15:18
@jaym jaym force-pushed the jdm/win-task-start-day branch from 37ab5f6 to 4097d64 Compare September 22, 2017 15:51
Jay Mundrawala added 5 commits September 25, 2017 05:50
DateTime.parse is not really that well defined. It accepts a lot
of different dates, however not `MM/DD/YYYY`. It does `DD/MM/YYYY`.
We should be very specific in the format we accept, and continue
to support `MM/DD/YYYY` as it was in the cookbook

Signed-off-by: Jay Mundrawala <jmundrawala@chef.io>
Signed-off-by: Jay Mundrawala <jmundrawala@chef.io>
Signed-off-by: Jay Mundrawala <jmundrawala@chef.io>
Signed-off-by: Jay Mundrawala <jmundrawala@chef.io>
Signed-off-by: Jay Mundrawala <jmundrawala@chef.io>
@jaym jaym force-pushed the jdm/win-task-start-day branch from 87d7731 to 5e13fe6 Compare September 25, 2017 12:50
@harikesh-kolekar
Copy link

@jaym If we fix the start_day format to mm/dd/yyyy, won't it break the windows_task resource for some users whose system date format wasn't mm/dd/yyyy? e.g. suppose the system date format for a particular region was dd/mm/yyyy and some users in that region were using windows_task resource with dd/mm/yyyy date format only. In this case, their recipes would have been working earlier but will break after this change.

@NimishaS
Copy link

Please note that windows_cookbook documentation also mentioned that the format for start_day should be mm/dd/yyyy: https://github.com/chef-cookbooks/windows#properties-12.
We can probably go with assumption that people should have been using the same format as documented.

@jaym
Copy link
Contributor Author

jaym commented Sep 25, 2017

If DD/MM/YYYY happened to work for those who used the windows cookbook, this change will break them. Since the documentation says MM/DD/YYYY, I think they were relying on a bug for things to work, so the question is do we want to be bug backwards compatible?

Copy link
Contributor

@tas50 tas50 left a comment

Choose a reason for hiding this comment

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

Seems like this might break users that were using the previous undocumented behavior, but that's the least bad end result.

@tas50 tas50 merged commit 128cc10 into master Sep 26, 2017
@Chef-Mike
Copy link

Chef-Mike commented Sep 27, 2017

I have some nodes affected by this. Will there be a new chef-client installer released?
or how can I get this resolved?
Do we need to increment metadata.rb on the chef-client cookbook to 8.1.9?

@tas50
Copy link
Contributor

tas50 commented Sep 27, 2017

If you want to give it a try checkout a build from our current channel at https://downloads.chef.io/chef/current

@tas50 tas50 deleted the jdm/win-task-start-day branch September 27, 2017 16:32
@Chef-Mike
Copy link

it works, thanks!

@lamont-granquist
Copy link
Contributor

ETA is most likely early next month for a release (we have a monthly release tempo for everything other than high severity security issues)

@spotlesscoder
Copy link

spotlesscoder commented Oct 19, 2017

Any chance this gets released earlier? That was already the third major problem I had with chef within the last 2 days.

The chef_client cookbook also is based on the windows cookbook. Due to that date format change, I could not bootstrap my Win2012R2 nodes in such a way that they make use of the chef-client recipe in my role which should have configured them in such a way that they would run chef-client every 5 minutes.

At the moment, I need to install the prerelease (13.5.22) version of chef-client manually on every bootstrapped node and run chef-client manually once more to make it work again. This just takes too much time.

@btm
Copy link
Contributor

btm commented Oct 19, 2017

@CodingSpiderFox how are you installing Chef? You could use knife bootstrap --bootstrap-version 13.5.22 --prerelease to install a specific version rather than installing the latest stable and reinstalling another version later.

The cadence for Chef releases is monthly. 13.5 was released 16 days ago, a bit late. The next 13 release should be happening soon.

@spotlesscoder
Copy link

Ah, I was missing the --prerelease, that's why it didn't work with specifying the 13.5.22 version.
Maybe knife could at least have warned me that I'm trying to install a prelease version and I need to add "--prerelease" to do so. Especially people new to chef don't know all the options

@spotlesscoder
Copy link

spotlesscoder commented Oct 19, 2017

@btm: It did not help to bootstrap with specifying v. 13.5.22.

knife bootstrap windows winrm <IP> -x '<DOMAIN>\<USER>' --bootstrap-proxy <PROXY> --bootstrap-no-proxy '<chef_server_ip>,<local_networks>' --run-list 'role[<ROLE>],' --node-ssl-verify-mode none --environment <ENVIRONMENT> --node-name <HOSTNAME> --bootstrap-version=13.5.22 --prerelease -V

The windows task for chef-client run is created but when it executes the first time after 5 minutes (and every further time), I get the following error:

(see attached client.log and stacktrace.out)

System language is English but date/time/currency etc. is German. OS ist Windows Server 2012 R2

client.log
stracktrace.txt

This has become a major issue for us.

Bootstrapping with the version 13.4 (resolved to 13.4.24) worked for us but the newer versions need to be fixed.

@btm
Copy link
Contributor

btm commented Oct 19, 2017

@CodingSpiderFox Can you please open a separate issue up and include a client log with -l debug? I'll get it on a team board soon.

@spotlesscoder
Copy link

So simply run the chef client manually with -l debug attached to the command I use?

@btm
Copy link
Contributor

btm commented Oct 19, 2017

@CodingSpiderFox yup! chef-client -l debug .... will produce much more verbose output and will save the developer working on it from having to ask a number of questions.

@spotlesscoder
Copy link

What kind of confidential data do I need to wipe out in the logs?

What about the X-Ops-Authorization-1 etc. headers? Should I remove them?

@spotlesscoder
Copy link

Opened issue: #6513

@chef chef locked and limited conversation to collaborators Feb 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants