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

Add get_properties() to modules/win_file.py #59770

Closed
wants to merge 3 commits into from
Closed

Add get_properties() to modules/win_file.py #59770

wants to merge 3 commits into from

Conversation

amalaguti
Copy link

What does this PR do?

Add get_properties() to modules/win_file.py

What issues does this PR fix or reference?

Fixes:
#59702

Previous Behavior

Function not present

New Behavior

This function returns Windows file properties

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

Yes/No

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@amalaguti amalaguti requested a review from a team as a code owner March 11, 2021 01:39
@amalaguti amalaguti requested review from twangboy and removed request for a team March 11, 2021 01:39
@xeacott
Copy link
Contributor

xeacott commented Mar 24, 2021

Thanks again for getting this PR up @amalaguti - as it currently stands, there's a couple more things we need to add this...

  1. changelog file
  2. run pre-commit on it
  3. add tests

1 and 2 should be relatively quick; generating a changelog file is as easy as just creating a file called 59888.added and then add a message that says add get_properties function to win_file.py and as for running pre-commit I can do that momentarily.

As for tests, you'll need to write some for tests/unit/modules/test_win_file.py so that we can ensure this behavior wont break in future changes. Unfortunately this file will also need to be converted into a pytest file and follow our pytest guidelines. If any of this sounds too much or if you don't have time, please let me know I'd be happy to help or show examples!

@amalaguti
Copy link
Author

@xeacott Joe, I'll need your help.
On creating the changelog, my doubt is why the file has to be named 59888.added (where is the 59888 coming from ? wondering if you mean 59770)

On the pre-commit, I'll leave it to you, thanks :)

About creating test, sorry not sure how to create one.

# backslash as parm returns dictionary of numeric info corresponding to VS_FIXEDFILEINFO struc
fixedInfo = win32api.GetFileVersionInfo(fname, '\\')
props['FixedFileInfo'] = fixedInfo
props['FileVersion'] = "%d.%d.%d.%d" % (fixedInfo['FileVersionMS'] / 65536,
Copy link
Contributor

Choose a reason for hiding this comment

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

These need to be string format instead of string substitution here to pass our linter.
So "string".format() instead of "%d (thing)".

strInfo[propName] = win32api.GetFileVersionInfo(fname, strInfoPath)

props['StringFileInfo'] = strInfo
except:
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, can you add the specific exception(s) that may be thrown? I believe fixedInfo = win32api.GetFileVersionInfo(fname, '\\') will at least throw a win32file.error.

@xeacott
Copy link
Contributor

xeacott commented Apr 5, 2021

Yeah! So sorry, meant 59702. It's the issue # not the PR number we're after. 😄 As for pre-commit ill run that on your branch and push up.

So as far as adding some tests to tests/unit/modules/test_win_file.py... a good idea would be to test that your new function is doing what its suppose to be doing. Something like this will probably be good enough, again just making sure that if someone modifies the function in the future, they aren't breaking anything that you established with it.

    def test_get_properties(self):
        """
        Test that given a file all properties return.
        """
        file = "C:\Program Files\Google\Chrome\Application\chrome.exe"

        expected = {{"permissions": {{'FixedFileInfo': None, 'StringFileInfo': None, 'FileVersion': None}} }
        ret = win_file.get_properties(fname=file)
        self.assertDictEqual(ret, expected)

@twangboy twangboy self-assigned this Sep 21, 2021
twangboy added a commit to twangboy/salt that referenced this pull request May 11, 2022
- Adds file.version that will get just the version
- Adds file.version_details to get additional information
- Adds tests
- Adds changelog

@amalaguti originally submitted a PR for this: saltstack#59770, but he has
since deleted his branch. Recognizing his contribution here.
Ch3LL pushed a commit that referenced this pull request May 23, 2022
- Adds file.version that will get just the version
- Adds file.version_details to get additional information
- Adds tests
- Adds changelog

@amalaguti originally submitted a PR for this: #59770, but he has
since deleted his branch. Recognizing his contribution here.
garethgreenaway pushed a commit to garethgreenaway/salt that referenced this pull request May 23, 2022
- Adds file.version that will get just the version
- Adds file.version_details to get additional information
- Adds tests
- Adds changelog

@amalaguti originally submitted a PR for this: saltstack#59770, but he has
since deleted his branch. Recognizing his contribution here.
Ch3LL pushed a commit that referenced this pull request May 26, 2022
* Making the retry state system feature available when parallel is set to True.

* swapping out using time.sleep Salt function for Python time.sleep.

* Skipping parallel tests on Windows.

* Fixing items, values, and keys functions in the data module.  Moving integration to functional tests.  Converting unit test over to pytest.

* adding changleog.

* fixes #62044 add ignore_missing to file.comment state

* fixes #61662 fix file.comment reports changes in test mode

* add 61662.fixed changelog

* add final test mode run after comment is successful

* fix uncomment_regex

* fix uncomment_regex

* Try not to trigger the GLIBC race condition.

See https://sourceware.org/bugzilla/show_bug.cgi?id=19329

Fixes #62071

Signed-off-by: Pedro Algarvio <[email protected]>

* Add missing CLI example to ``network.fqdns``

Signed-off-by: Pedro Algarvio <[email protected]>

* Sleep between submitting new jobs to thread pool

Signed-off-by: Pedro Algarvio <[email protected]>

* Sleep before trying to resolve instead

Signed-off-by: Pedro Algarvio <[email protected]>

* Shorter sleeps

* fixes #61944 fixed backslash literal bytestring

* When states are running in parallel, ensure that the total run time produced by the highstate outputter takes that into account.

* removing unrelated change

* Adding changelog.

* Skip parallel test on Windows.

* skip pdbedit unit tests on ubuntu 2204 because md4 is not supported as a hash type

* refresh db at the beginning for destructive aptpkg functional tests

* fix failing test_pkgrepo_managed_absent and actually capture and keep signed-by information in salt/modules/aptpkg.py custom implementation of apt repository management

* Update tests/unit/modules/test_pdbedit.py

Co-authored-by: Pedro Algarvio <[email protected]>

* Disable hack to force allowing forking on macOS

* Use the newer nox platforms file which installs Py3.9

Signed-off-by: Pedro Algarvio <[email protected]>

* Support macOS defaulting to spawn as the default multiprocessing method

* Remove unused attribute

* Switch to ``salt.utils.platform.spawning_platform()``

Signed-off-by: Pedro Algarvio <[email protected]>

* Reconstruct client instances on spawning platforms

* Also skip this test for now on macOS

Signed-off-by: Pedro Algarvio <[email protected]>

* Reconstruct the ``State`` class on spawned processes.

Signed-off-by: Pedro Algarvio <[email protected]>

* Force a reference to ``salt.ext.tornado.iostream.StreamClosedError`` to avoid ``NameError`` issues

Signed-off-by: Pedro Algarvio <[email protected]>

* Skip, for now, problematic tests on spawning platforms

Signed-off-by: Pedro Algarvio <[email protected]>

* DeltaProxy minions do not work on spawning platforms

Signed-off-by: Pedro Algarvio <[email protected]>

* Avoid a ``NameError`` when failing to initialize the process list

Signed-off-by: Pedro Algarvio <[email protected]>

* Use Python 3.9 on macOS

Signed-off-by: Pedro Algarvio <[email protected]>

* Drop requirements on Darwin for Py<3.9

Signed-off-by: Pedro Algarvio <[email protected]>

* Same test run timeout as windows

Signed-off-by: Pedro Algarvio <[email protected]>

* Don't shell out! Access the database directly and properly escape values.

Signed-off-by: Pedro Algarvio <[email protected]>

* Fixes for the new macOS Catalina and BigSur CICD images

Signed-off-by: Pedro Algarvio <[email protected]>

* Migrate ``integration.modules.test_mac_pkgutil`` to PyTest and functional tests.

Fixes #60819

Signed-off-by: Pedro Algarvio <[email protected]>

* Add changelog for #57742

Fixes #57742

Signed-off-by: Pedro Algarvio <[email protected]>

* Add changelog entry.

Fixes #55847

Signed-off-by: Pedro Algarvio <[email protected]>

* initial Proxy Module

* inital pass of restconf states

* adding codeauthor

* adding restconf module

* doc update

* doc update

* doc update

* add requirements chefk to states module

* doc update

* doc update

* doc update

* add changelog entry

* Update salt/proxy/restconf.py

Co-authored-by: Wayne Werner <[email protected]>

* Update salt/proxy/restconf.py

Co-authored-by: Wayne Werner <[email protected]>

* remove noqa statements

* docs update & opts defaults

* docs update

* error checks + https transport method

* docs update

* docs update

* doing a fix for commit check

* adding test and updating modules

* removing old code

* DRY

* docs update

* output updates

* prepping for deepdiff switch

* states testing

* more tests

* changed changelog from fixed to added

* force pre-commit checks

* new style of diff output that is readable

* updating tests and output

* updating output style to make YAML diff more readable

* add test for restconf module

* adjusting based on pr review

* adjusting based on pr review

* adjusting true/false verify based on pr review

* change uri to path and clean args list

Co-authored-by: Wayne Werner <[email protected]>

* adjusting uri to path based on pr review

* adjusting capabilities path based on pr review

* Update salt/modules/restconf.py

Co-authored-by: Wayne Werner <[email protected]>

* adjusting val based on pr review

* caps to confirm to RFC 8040

* WIP: refactor based on feedback

* WIP: changed results to be any kind of falsey val

* linting - pre-commit

* updating docs

* Update salt/states/restconf.py

Co-authored-by: Wayne Werner <[email protected]>

* updating style

* fixes for lint and nox pass

* fixing

* blacken

* Update restconf.py

* add cli examples

* change logging type

* add pytest skip for ordereddict issue with legacy python

* add sys library for pytest mark skipif check

* pre-commit fixes

* Update salt/proxy/restconf.py

Co-authored-by: Pedro Algarvio <[email protected]>

* Update salt/states/restconf.py

Co-authored-by: Pedro Algarvio <[email protected]>

* Update tests/pytests/unit/modules/test_restconf.py

Co-authored-by: Pedro Algarvio <[email protected]>

* Update tests/pytests/unit/states/test_restconf.py

Co-authored-by: Pedro Algarvio <[email protected]>

* Update tests/pytests/unit/proxy/test_restconf.py

Co-authored-by: Pedro Algarvio <[email protected]>

* pre-commit fix

* update logging

* Update restconf.py

* some rest apis dont follow the standard correctly (cisco) and need another header

* fixes #61946 sync_after_install immutabledict error

* add configurable sync sleep for create

* add testing around the particular sync command called

* Adds the ability to get file version information on Windows

- Adds file.version that will get just the version
- Adds file.version_details to get additional information
- Adds tests
- Adds changelog

@amalaguti originally submitted a PR for this: #59770, but he has
since deleted his branch. Recognizing his contribution here.

* Update states.chef for version 16.x and 17.x Chef Infra Client output

* Support previous Chef version plus version 16.x and 17.x output.

* Remove legacy chef unit test

* Add unit tests for chef state

These tests cover the entire state, but are only concerned with the
ret["result"] of the outputs. It would be a good idea to test other
outputs as well - these tests could be modified to include assertions
about the other parts of ret. (Obviously they would need to be slightly
renamed. Maybe to `..._return_expected_ret`

* Update salt-bootstrap to 2022.03.15 release

* Update state.py

Fixing lint.

* Running pre-commit manually.

* swap out salt.utils.platform.is_windows for salt.utils.platform.spawning_platform.  Bump up duration for test_retry_option_success_parallel to 30 seconds to account for test running on Mac OS.

Co-authored-by: Megan Wilhite <[email protected]>
Co-authored-by: nicholasmhughes <[email protected]>
Co-authored-by: Pedro Algarvio <[email protected]>
Co-authored-by: emmadionne1 <[email protected]>
Co-authored-by: MKLeb <[email protected]>
Co-authored-by: Caleb Beard <[email protected]>
Co-authored-by: Pedro Algarvio <[email protected]>
Co-authored-by: Jamie Murphy <[email protected]>
Co-authored-by: Jamie (Bear) Murphy <[email protected]>
Co-authored-by: Jamie Murphy <[email protected]>
Co-authored-by: Wayne Werner <[email protected]>
Co-authored-by: Cassandra Durnford <[email protected]>
Co-authored-by: Jamie Murphy <[email protected]>
Co-authored-by: Jamie (Bear) Murphy <[email protected]>
Co-authored-by: Twangboy <[email protected]>
Co-authored-by: Eric Ham <[email protected]>
Co-authored-by: Wayne Werner <[email protected]>
Co-authored-by: krionbsd <[email protected]>
@twangboy
Copy link
Contributor

These changes were added with #62054

@twangboy twangboy closed this Sep 21, 2022
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.

4 participants