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

[Utility] Added option to select 7z level of compression. #7058

Merged
merged 2 commits into from
Sep 25, 2019

Conversation

alexwiese
Copy link
Contributor

Similarly to the tarCompression option, this change allows the user to select the level of compression for 7z files.

The option names follow the naming conventions used by 7z.

@msftclas
Copy link

msftclas commented Apr 24, 2018

CLA assistant check
All CLA requirements met.

@alexwiese alexwiese changed the title Added option to select 7z level of compression. [Utility] Added option to select 7z level of compression. Apr 24, 2018
@alexwiese
Copy link
Contributor Author

@davidstaheli

@alexwiese alexwiese force-pushed the 7z-compression-level branch 4 times, most recently from 5bcd0bd to 5f63ebe Compare April 29, 2018 23:03
@alexwiese
Copy link
Contributor Author

@brcrista

@davidstaheli
Copy link
Contributor

@alexwiese thanks for this great contribution! Nice work. Two thoughts:

  1. Can you please change the minor version from 128 to 135? This number corresponds with our sprint number, allowing us to track when changes are made.
  2. What do you think about changing the option IDs from numeric ("0", "1", "3", "5", "7", "9") to their corresponding name ("normal", "fast", etc.)? That way, when this task is used in a YAML file definition, it will be more readable, like:
- task: ArchiveFiles@2
  inputs:
    rootFolderOrFile: '$(system.defaultWorkingDirectory)'
    archiveType: 7z
    sevenZipCompression: maximum

Instead of:

- task: ArchiveFiles@2
  inputs:
    rootFolderOrFile: '$(system.defaultWorkingDirectory)'
    archiveType: 7z
    sevenZipCompression: 7

@@ -79,6 +79,12 @@ function sevenZipArchive(archive: string, compression: string, files: string[])
var sevenZip = tl.createToolRunner(getSevenZipLocation());
sevenZip.arg('a');
sevenZip.arg('-t' + compression);

const sevenZipCompression = tl.getInput('sevenZipCompression', false);
if(sevenZipCompression) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add space after if

@brcrista
Copy link
Contributor

Looks good to me, but I don't own this task. I'll let @bryanmacfarlane or @ericsciple complete the merge if they agree with adding this parameter.

@alexwiese alexwiese force-pushed the 7z-compression-level branch from 5f63ebe to b259644 Compare April 30, 2018 22:17
@alexwiese
Copy link
Contributor Author

alexwiese commented Apr 30, 2018

@davidstaheli @brcrista thanks. Makes sense. Please see latest commit.

@davidstaheli
Copy link
Contributor

For safety with values people type in YAML, you could call toLower() on sevenZipCompression before switching on it.

@alexwiese alexwiese force-pushed the 7z-compression-level branch from b259644 to 3915de1 Compare May 1, 2018 00:13
@alexwiese
Copy link
Contributor Author

This should be ready to merge now.

@alexwiese alexwiese force-pushed the 7z-compression-level branch 2 times, most recently from f995f0b to 7979b90 Compare May 8, 2018 23:46
@alexwiese alexwiese force-pushed the 7z-compression-level branch 3 times, most recently from 6366e41 to 6331c24 Compare May 20, 2018 23:24
@damccorm
Copy link

@alexwiese sorry this slipped through the cracks! If you're still interested, I think we should take this. If you can resolve conflicts and update the task.json/task.loc.json to 2.159.0 I'll merge.

@alexwiese
Copy link
Contributor Author

@damccorm done :)

@damccorm damccorm merged commit 699a04b into microsoft:master Sep 25, 2019
@damccorm
Copy link

Merged! Thanks for the really quick turnaround 😃

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.

5 participants