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

btrfs: Add properties state #52699

Merged
merged 8 commits into from
Apr 22, 2020
Merged

btrfs: Add properties state #52699

merged 8 commits into from
Apr 22, 2020

Conversation

aplanas
Copy link
Contributor

@aplanas aplanas commented Apr 25, 2019

What does this PR do?

We can set via btrfs property command some properties. This command
is used in the btrfs execution module.

The current state will leverate this code to make sure that some
properties al set or unset in a safe mode.

Tests written?

Yes

@aplanas
Copy link
Contributor Author

aplanas commented May 6, 2019

What is the opinion about the code climate complains? Some of them are weird to me.

@aplanas
Copy link
Contributor Author

aplanas commented May 10, 2019

Ready for reviews : )

@twangboy twangboy requested a review from a team May 13, 2019 22:17
@waynew
Copy link
Contributor

waynew commented May 16, 2019

@aplanas Yeah, we're (gradually) tuning the checks. Most of them are a little aggressive for what we need. We take the advice with a heaping spoonful of salt :trollface:

@aplanas
Copy link
Contributor Author

aplanas commented Oct 14, 2019

Rebased on top of #54962, that is also rebased on master. Once the other one is merged, I can rebase on top of the real master.

@aplanas aplanas force-pushed the fix_brtfs branch 2 times, most recently from 450d104 to 1c69723 Compare April 9, 2020 13:01
@waynew
Copy link
Contributor

waynew commented Apr 14, 2020

Just merged master in - twice it was missing ERROR: /var/lib/jenkins/workspace/pr-pre-commit_PR-52699@script/.ci/pre-commit not found so I think it was missing the ci file that Pedro added a couple of days ago. 🤞

@waynew
Copy link
Contributor

waynew commented Apr 14, 2020

@aplanas looks like it needs some pre-commit hooks executed. If you haven't yet, quickstart docs has a guide on setting it up.

@aplanas aplanas force-pushed the fix_brtfs branch 2 times, most recently from 441a4d0 to e59e156 Compare April 15, 2020 10:44
@aplanas
Copy link
Contributor Author

aplanas commented Apr 15, 2020

The fails now is about missing documentation. How can I generate it?

@waynew
Copy link
Contributor

waynew commented Apr 17, 2020

Did you get the docs figured out? I'm not seeing any failure messages about that (though we did recently merge a PR that would fail if the module lacks an entry in the docs/ folder.

I am seeing this failure, which looks definitely related: https://jenkinsci.saltstack.com/job/pr-fedora30-py3/job/PR-52699/27/testReport/junit/unit.states.test_btrfs/BtrfsTestCase/test_properties_test/

aplanas and others added 8 commits April 17, 2020 16:24
(cherry picked from commit 237d40d)
When we want to create a subvolume and set is as default, we can
use the set_default parameter. The current code will check, for
already created subvolumes, if it is actually marked as default,
and if not it will be set inside the state.

For composability with other states, we sometimes want to avoid
the re-set of the default flag for already present subvolumes, as
can be that this was change in a later action by another state.

(cherry picked from commit 067482e)
The decorator can work without a device (so nothing can be mounted)

Also the traceback removed in PR#51205 is re-added, to show the
internal exception from the decorated code.
We can set via `btrfs property` command some properties. This command
is used in the btrfs execution module.

The current state will leverate this code to make sure that some
properties al set or unset in a safe mode.

For example:

   set_property_ro_/:
     - name: /
     - device: /dev/sda1
     - use_defaults: yes
     - ro: yes
@aplanas
Copy link
Contributor Author

aplanas commented Apr 17, 2020

Yes, documentation issue figure out. I fixed the test case, was a bad rebase.

@dwoz
Copy link
Contributor

dwoz commented Apr 20, 2020

@waynew Look good now?

Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

@dwoz LGTM 👍

@dwoz dwoz merged commit a9ebb98 into saltstack:master Apr 22, 2020
@aplanas aplanas deleted the fix_brtfs branch April 23, 2020 07:56
@sagetherage sagetherage added the ZRelease-Sodium retired label label May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants