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.

Sign up for GitHub

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

Jump to bottom

Name things once #50

Merged
merged 1 commit into from Apr 15, 2021
Merged

Name things once #50

merged 1 commit into from Apr 15, 2021

Conversation

s0undt3ch
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Apr 14, 2021 •
edited

Codecov Report

Merging #50 (19667c5) into master (57c903e) will decrease coverage by 0.09%.
The diff coverage is 89.98%.

❗ Current head 19667c5 differs from pull request most recent head cb2646a. Consider uploading reports for the commit cb2646a to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master      #50      +/-   ##
==========================================
- Coverage   87.01%   86.91%   -0.10%     
==========================================
  Files         131      131              
  Lines        7192     7214      +22     
  Branches      844      844              
==========================================
+ Hits         6258     6270      +12     
- Misses        671      681      +10     
  Partials      263      263
Flag Coverage Δ
86.31% <89.78%> (-0.10%) ⬇️
Linux 85.66% <89.39%> (-0.13%) ⬇️
factories 86.20% <89.78%> (-0.10%) ⬇️
macOS 82.04% <87.67%> (-0.09%) ⬇️
py36 83.60% <88.07%> (-0.10%) ⬇️
py37 83.65% <88.27%> (-0.10%) ⬇️
py38 85.63% <89.39%> (-0.10%) ⬇️
py39 84.13% <88.21%> (-0.12%) ⬇️
salt_3002_6 84.19% <88.40%> (-0.12%) ⬇️
system 44.71% <49.50%> (+0.03%) ⬆️
tests 34.50% <25.21%> (+0.15%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/saltfactories/__main__.py 81.81% <ø> (ø)
src/saltfactories/cli/__init__.py 100.00% <ø> (ø)
src/saltfactories/daemons/__init__.py 100.00% <ø> (ø)
...s/functional/markers/test_requires_salt_modules.py 100.00% <ø> (ø)
...ts/functional/markers/test_requires_salt_states.py 100.00% <ø> (ø)
...ntegration/factories/daemons/syndic/test_syndic.py 31.03% <0.00%> (ø)
src/saltfactories/daemons/container.py 54.25% <46.29%> (ø)
src/saltfactories/cli/cloud.py 89.79% <72.72%> (ø)
...s/functional/factories/base/test_daemon_factory.py 85.63% <78.57%> (ø)
src/saltfactories/bases.py 87.27% <90.90%> (ø)
... and 55 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 57c903e...cb2646a. Read the comment docs.

@s0undt3ch s0undt3ch force-pushed the hotfix/name-things-once branch 2 times, most recently from 0bdb9ed to bf5f855 Compare April 14, 2021 15:21
@s0undt3ch s0undt3ch requested a review from dwoz April 14, 2021 15:22
@s0undt3ch s0undt3ch force-pushed the hotfix/name-things-once branch 2 times, most recently from a7731f2 to ce4f865 Compare April 14, 2021 15:40
@saltstack saltstack deleted a comment from lgtm-com bot Apr 14, 2021
@saltstack saltstack deleted a comment from lgtm-com bot Apr 14, 2021
@saltstack saltstack deleted a comment from lgtm-com bot Apr 14, 2021
@s0undt3ch s0undt3ch marked this pull request as ready for review April 14, 2021 15:41
@s0undt3ch s0undt3ch force-pushed the hotfix/name-things-once branch 2 times, most recently from df7c526 to a2cc433 Compare April 14, 2021 16:08
@saltstack saltstack deleted a comment from lgtm-com bot Apr 14, 2021
@saltstack saltstack deleted a comment from lgtm-com bot Apr 14, 2021
@s0undt3ch s0undt3ch force-pushed the hotfix/name-things-once branch 2 times, most recently from 3d7c888 to 2a5e917 Compare April 14, 2021 18:41
@saltstack saltstack deleted a comment from lgtm-com bot Apr 14, 2021
dwoz
dwoz reviewed Apr 14, 2021
View reviewed changes
Copy link

@dwoz dwoz left a comment

Choose a reason for hiding this comment

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

Take everything I say here with a grain of salt (pun intended). Nice to see these changes overall! :)

src/saltfactories/cli/cloud.py Outdated
from saltfactories.utils import running_username

log = logging.getLogger(__name__)


@attr.s(kw_only=True, slots=True)
class SaltCloudFactory(SaltCliFactory):
class SaltCloud(SaltCli):
@staticmethod
def default_config(root_dir, master_id, config_defaults=None, config_overrides=None):
Copy link

Choose a reason for hiding this comment

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

By being the only config method it implies that it is the default. Since the name of of the method has config in it, defaults and overrides can be inferred to be config. I think this could be simplified to.

def config(root_dir, master_id, defaults, overrides):

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, we can't just rename that function, it will shaddow the self.config dictionary which is the end result of all configure steps.
So, either keep the function name as default_config or generate_config?! defaults()?

Copy link

Choose a reason for hiding this comment

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

Keep it as default_config :)

src/saltfactories/cli/cloud.py
from saltfactories.utils import running_username

log = logging.getLogger(__name__)


@attr.s(kw_only=True, slots=True)
class SaltCloudFactory(SaltCliFactory):
class SaltCloud(SaltCli):
Copy link

Choose a reason for hiding this comment

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

Docstrings for the methods on this class would be helpful.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed

src/saltfactories/daemons/container.py Outdated Show resolved Hide resolved
src/saltfactories/daemons/master.py Outdated Show resolved Hide resolved
tests/unit/factories/base/test_salt_cli_factory.py Outdated Show resolved Hide resolved
tests/unit/factories/cli/test_salt.py Outdated Show resolved Hide resolved
@s0undt3ch s0undt3ch force-pushed the hotfix/name-things-once branch 2 times, most recently from 15c058e to 19667c5 Compare April 15, 2021 11:05
@saltstack saltstack deleted a comment from lgtm-com bot Apr 15, 2021
@saltstack saltstack deleted a comment from lgtm-com bot Apr 15, 2021
@saltstack saltstack deleted a comment from lgtm-com bot Apr 15, 2021
@saltstack saltstack deleted a comment from lgtm-com bot Apr 15, 2021
@saltstack saltstack deleted a comment from lgtm-com bot Apr 15, 2021
@saltstack saltstack deleted a comment from lgtm-com bot Apr 15, 2021
@saltstack saltstack deleted a comment from lgtm-com bot Apr 15, 2021
@lgtm-com
Copy link

lgtm-com bot commented Apr 15, 2021

This pull request fixes 1 alert when merging cb2646a into 57c903e - view on LGTM.com

fixed alerts:

  • 1 for Conflicting attributes in base classes

@s0undt3ch
Copy link
Member Author

🎉

@s0undt3ch s0undt3ch merged commit 0485d83 into saltstack:master Apr 15, 2021
@s0undt3ch s0undt3ch deleted the hotfix/name-things-once branch April 15, 2021 19:41
s0undt3ch added a commit to s0undt3ch/pytest-salt-factories that referenced this pull request Jun 17, 2021
@s0undt3ch
Rename SaltMaster.get_salt_cli to SaltMaster.salt_cli
393dac9 
Forgotten in saltstack#50
@s0undt3ch s0undt3ch mentioned this pull request Jun 17, 2021
Merged
s0undt3ch added a commit that referenced this pull request Jun 17, 2021
@s0undt3ch
Rename SaltMaster.get_salt_cli to SaltMaster.salt_cli
550122c 
Forgotten in #50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dwoz dwoz dwoz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@s0undt3ch @dwoz