Fix Issue#55

[skyguy94]

This PR fixes issue #55 by changing any year that occurs before 1980 to 1980 so that the archive gets created. It also outputs a warning about the date change during compression.

[KirkMunro]

Thank you for submitting this PR.

Before providing my review, I would like to hear from some others about the proposed changes.

@SteveL-MSFT: The APIs used to create an archive document the DateTime limits on the LastWriteTime attribute here. In the event when older documents are added to an archive, do we simply want to move forward their timestamp to within the allowed range, with a warning as is being done in this PR? I think it's a good compromise given the limits of the API being used for compression, although I would suggest the warning text should change to more clearly identify why it is warning the user, and I think the date should be set to the earliest possible date rather than just changing the year, but I'll handle those in a review after I hear your thoughts on this approach to this issue.

[SteveL-MSFT]

Given the limits of the api, I think a warning is sufficient. Setting it to the earliest allowed date seems reasonable to me.

[skyguy94]

Thanks guys. I wasn't sure what to do exactly or what to log as I don't think this issue is all that straightforward to try and handle. The least complex thing I could think of was to bump up the date and warn.

I updated the PR to use [DateTime]::Parse and an ISO 8601 date with the default value in whatever the local time is for the machine performing the archiving. If you have any ideas about improving the log message let me know.

[KirkMunro]

Thanks Ritch. This is looking good. I have two main thoughts after reviewing the latest changes:

1. In order to accept this PR, we need Pester tests added to the Pester test file to make sure this works and is properly tested with Pester. Tests should specifically include files after 1980, where you validate that the LastWriteTime of the file before/after archiving (after extraction) is the same as it was in the beginning, and files before 1980, where you validate that the LastWriteTime after archiving is set to 1980-01-01T00:00:00. Make sense?

2. The warning message should do more than just tell the end user when a file has a LastWriteTime earlier than 1980. It should tell them why that is a warning. I also think the path should be in single quotes since it may contain spaces. Maybe something more like this:

   Write-Warning "'$currentFilePath' has LastWriteTime earlier than 1980. Any files with LastWriteTime values earlier than 1980 will be stored in the archive with a LastWriteTime of 1980-01-01T00:00:00."

Would you please make those additional changes and then update the PR?

[msftclas]

All CLA requirements met.

[KirkMunro]

Changes look good. I like the warning message. Note that when setting $lastWriteTime using [DateTime]::Parse, you can drop everything from the T onward since it is not adding anything, e.g. $lastWriteTime = [DateTime]::Parse('1980-01-01').

The only thing missing from your changes is one more Pester test that does the following: takes two files, one with a last write time earlier than 1980, another with a last write time post 1980, compress them into an archive using Compress-Archive, extract them from the archive into a different folder using Expand-Archive, and then verify the last write time after they are expanded. For the file with the last write time pre 1980, it should end up with a last write time of 1980-01-01T00:00:00. For the other file, the last write time should be the same as it was before archiving.

[skyguy94]

Ok, that test makes sense. For the parse, I can change it but I felt that being explicit about the time being the local 00:00 time. Without being explicit I felt it was unclear about zulu vs local time while reading the line.

[KirkMunro]

I'm ok with the explicit parse. You're right, it's less ambiguous.

[skyguy94]

I think there's something different about how LastWriteTime gets stored or set when expanded. If I compare the expanded to the source time the test generally fails with the expanded time having a seconds field 1 set to one second earlier than the original source file (yes, earlier).

I chose to just go with an inverted condition (it should not be the default date) for the non-modified file. I think if your really want a DateTime compare, then we'll have to go with the typical 'RoundedToXXXX' pattern that requires constructing a new DateTime object from the old without the rounded fields. In this case it would be seconds. I can do that as a helper method in the file, but it seemed like a bit much for a condition not really relevant to what we are trying test.

[KirkMunro]

Given the rounding issue, I like your approach. The key point with this test was to verify that files with old dates had their dates properly updated post Expand-Archive, and that files with newer dates were unchanged, and using an inverted condition does that. Thanks for making those additional changes, Ritch! I'll do my final review now.

[KirkMunro]

Why Write-Output here? Was this put in for testing/troubleshooting and then left behind?

[skyguy94]

Oops. yeah

[KirkMunro]

LGTM. Merging...