minion_id_remove_domain could contain string and bool values #49378

marbx · 2018-08-28T19:00:57Z

This PR replaces PR #49213

What does this PR do?

When a minion lacks its minion_id at startup, salt.config.__init__.py requests one from utils.network.generate_minion_id(), which returns the fully qualified domain name (FQDN) of the host.

When all minions are within one domain and named after their hostnames, generated a "FQDN minion id" would be an inconsistent naming.

salt.config.__init__.py has the option to lowercase the minion_id.
salt.config.__init__.py also has the option to append a domain to a generated "hostname minion id".

I hereby propose a new option, to generate a "hostname minion id", by removing a single domain from a FQDN.

What issues does this PR fix or reference?

Issue #49212

Previous Behavior (example)

The generated minion_id is king_bob.foo.org

New Behavior (example)

With minion_id_remove_domain: False, as previously.
With minion_id_remove_domain: bar.org, as previously.
With minion_id_remove_domain: foo.org, the generated minion_id becomes king_bob
With minion_id_remove_domain: True, the generated minion_id becomes king_bob

Tests written?

Yes

…kuskramerIgitt/salt into minion_id_generate_no_domain

DmitryKuzmenko

LGTM, I'd just like to see some minor changes in there.

salt/config/__init__.py

isbm

I am approving this, thanks @markuskramerIgitt ! However, I have just one suggestion to rephrase your table-list schematic example into something generic, typically readable. I include my suggestion.

doc/ref/configuration/minion.rst

marbx · 2018-08-29T12:57:27Z

Hello @isbm, the page states you approved my changes. Why does the page still shows "Changes requested"?

terminalmage

We need test cases for the True and False values for this config option. Also, is this intended to replace #49213? If so, we should probably close #49213.

doc/ref/configuration/minion.rst

salt/config/__init__.py

isbm · 2018-08-29T15:37:18Z

@markuskramerIgitt because @terminalmage needs unit tests (+1, BTW) and some minor fixes. 😉

terminalmage · 2018-08-29T16:00:16Z

Actually, I think it is because @DmitryKuzmenko had requested changes and has not re-reviewed with an Approved outcome, because @markuskramerIgitt's question pre-dated my review.

DmitryKuzmenko · 2018-08-29T21:58:50Z

who? me? sorry, done. =-/

marbx · 2018-08-30T08:48:53Z

Of the two PR...

Option to remove domain in the generated minion_id #49213 is string type and complete
minion_id_remove_domain could contain string and bool values #49378 is (string,bool) type, needs refactoring due to cyclomatic complexity 22 in get_id(), boolean test cases and a more serene description for boolean True.

… only one is necessary (they are mutually exclusive).
Either one is fine for me

@terminalmage, which do you like? Can you decide alone?

@isbm, if #49378 is favored, could you please change/add the Boolean part? I see that you have more experience in Python.

isbm · 2018-08-30T10:00:08Z

[...] Can you decide alone?

So everyone can blame @terminalmage for this? 😄 In case #49378 is there, I'd like to be blamed too!

This boils down that while any domain cut would be indeed better user experience for small deployments, question is should we introduce some new tweaks and knobs to existing pile, or just reuse what already is there. My personal opinion is to reuse. In Salt we already doing this all over the place, so there is no reason that this time it needs to be any different. Also if the directive is to how to remove domain that refers to the same action, I see no reason to have two separate options doing conceptually the same thing.

But I also think that @terminalmage still possibly can find me wrong here, so objections to the above are welcome.

@markuskramerIgitt what exactly should I change?

terminalmage · 2018-08-30T19:12:23Z

I like this approach better than the other PR, but the changes I requested still need to be made before this PR is ready for merge.

marbx · 2018-09-01T08:58:46Z

@terminalmage, thank you for your vote, I see there are more examples of "mixed type" in the same module. Have I now completed this PR?

I think both PR #49213 and #49378 are ready for merge.
Please accept one and close the other.

marbx · 2018-09-04T20:34:55Z

Hi @terminalmage, belowit says that you "request changes", but I don't find which ones.
Which changes do you request?

I also don't understand which checks exactly are "not successful".

marbx · 2018-10-04T11:44:24Z

Hi @isbm,
the above exception is thrown by salt/_compat.py#L194

        try:
            packed = bool(int(str(bytearray(data)).encode('hex'), 16))
        except ValueError:

This is intended as a test if data is a packed binay.
I would add TypeError to the known exception list.

10 days ago you altered this line - what do you think?

Thank you,
Markus

isbm · 2018-10-05T08:44:50Z

@markuskramerIgitt hmm, why do we have there TypeError? What you're getting into data variable then?

isbm · 2018-10-05T10:04:22Z

@markuskramerIgitt oh OK, I know why. You found a bug, congrats. You also found a correct solution (I've debugged it and think the same). 🍿

PR to fix this: #49908

rallytime · 2018-10-05T13:19:51Z

Nice - thanks for helping out here @isbm!

marbx · 2018-10-05T17:46:19Z

Hi @isbm, thank you for picking it up.

marbx · 2018-10-05T17:48:46Z

Jenkins still fails, but for no (apparent) reason?

Is there something I can do here?

rallytime · 2018-10-05T18:09:44Z

@markuskramerIgitt We're working through an issue with our pip installer right now, so no need to worry here right now. We'll get these going again when the issue is resolved. Thanks!

marbx · 2018-10-25T15:58:06Z

@rallytime, thank you for merging.
How will this pull request make its way from 'develop' into a release?

gtmanfred · 2018-10-25T16:02:40Z

This will be in the Neon feature release.

marbx · 2018-10-26T08:46:24Z

Thank you, now I unterstand versionadded:: Neon.
Neon comes after Fluorine

marbx · 2019-05-24T19:10:14Z

@gtmanfred, could you please consider to merge this change into a Fluorine release?
Otherwise I would need to create a local modification.

marbx · 2019-05-24T21:34:54Z

@gtmanfred, please forget to consider merging at an earlier time.
I just found that I can help myself with id_function, no local modification needed.

marbx · 2019-09-18T15:37:38Z

As of today, Neon still has no release version.

marbx · 2019-09-19T13:05:59Z

@gtmanfred, please forget to consider merging at an earlier time.
I just found that I can help myself with id_function, no local modification needed.

Unfortunately: id_function, requires configuration of the salt master and of the (to be modified) salt-minion installer:

The installers must configure id_function before the minion service starts, and the salt-master must not remove this function, later.

[master] Porting #49378 to master

marbx · 2020-03-11T21:52:08Z

minion_id_remove_domain is not in git checkout v2019.2.1 and git checkout v2019.2.3 but in

git checkout v2019.8
git checkout v3000

marbx and others added 11 commits August 20, 2018 20:59

Issue #49212

21ebbb2

Issue #49212

b371a3e

Issue #49212

b70a401

Issue #49212

a8792d4

Remove trailing whitespace

a8b79f7

Naming symmetrical to append_domain

6965455

Merge conflict (chomped whitespace) resolved

6eef007

Merge branch 'develop' into minion_id_generate_no_domain

4fde9af

Renamed minion_id_remove_domain

1ec55ca

Merge branch 'minion_id_generate_no_domain' of https://github.com/mar…

c3846a4

…kuskramerIgitt/salt into minion_id_generate_no_domain

Skeleton

bb0e0ad

marbx requested a review from a team as a code owner August 28, 2018 19:00

ghost self-requested a review August 28, 2018 19:00

DmitryKuzmenko suggested changes Aug 28, 2018

View reviewed changes

salt/config/__init__.py Outdated Show resolved Hide resolved

salt/config/__init__.py Outdated Show resolved Hide resolved

salt/config/__init__.py Outdated Show resolved Hide resolved

salt/config/__init__.py Outdated Show resolved Hide resolved

marbx added 2 commits August 28, 2018 22:52

Minor changes from @isbm

9275491

Documentation

999efca

isbm approved these changes Aug 29, 2018

View reviewed changes

doc/ref/configuration/minion.rst Show resolved Hide resolved

terminalmage suggested changes Aug 29, 2018

View reviewed changes

doc/ref/configuration/minion.rst Outdated Show resolved Hide resolved

salt/config/__init__.py Outdated Show resolved Hide resolved

DmitryKuzmenko approved these changes Aug 29, 2018

View reviewed changes

Reduced cyclomatic complexity, added 3 test cases and documentation

e0c9643

Bugfix refactored function needs opts variable

949cab8

isbm mentioned this pull request Oct 5, 2018

Bugfix: any unicode string of length 16 will raise TypeError #49908

Merged

rallytime closed this in #49908 Oct 5, 2018

rallytime reopened this Oct 5, 2018

Merge branch 'develop' into minion_id_remove_domain_as_mixed_type

b942c9f

Merge branch 'develop' into minion_id_remove_domain_as_mixed_type

1b3a734

rallytime merged commit fe2713e into saltstack:develop Oct 9, 2018

marbx deleted the minion_id_remove_domain_as_mixed_type branch October 25, 2018 15:54

marbx mentioned this pull request Sep 18, 2019

Add setting minion_id_remove_domain marbx/salt-windows-msi#68

Closed

garethgreenaway added a commit to garethgreenaway/salt that referenced this pull request Sep 19, 2019

Porting PR saltstack#49378 to 2019.2.1

dc84cb3

garethgreenaway mentioned this pull request Sep 19, 2019

[master] Porting #49378 to master #54622

Merged

dwoz added a commit that referenced this pull request Dec 6, 2019

Merge pull request #54622 from garethgreenaway/2019_2_1_port_49378

a01b7f5

[master] Porting #49378 to master

marbx mentioned this pull request Jan 10, 2021

minion id ends with non-existing domain ".mshome.net" #52162

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

minion_id_remove_domain could contain string and bool values #49378

minion_id_remove_domain could contain string and bool values #49378

marbx commented Aug 28, 2018 •

edited

Loading

DmitryKuzmenko left a comment

isbm left a comment

marbx commented Aug 29, 2018

terminalmage left a comment

isbm commented Aug 29, 2018

terminalmage commented Aug 29, 2018 •

edited

Loading

DmitryKuzmenko commented Aug 29, 2018

marbx commented Aug 30, 2018

isbm commented Aug 30, 2018 •

edited

Loading

terminalmage commented Aug 30, 2018

marbx commented Sep 1, 2018 •

edited

Loading

marbx commented Sep 4, 2018 •

edited

Loading

marbx commented Oct 4, 2018

isbm commented Oct 5, 2018

isbm commented Oct 5, 2018 •

edited

Loading

rallytime commented Oct 5, 2018

marbx commented Oct 5, 2018

marbx commented Oct 5, 2018

rallytime commented Oct 5, 2018

marbx commented Oct 25, 2018

gtmanfred commented Oct 25, 2018

marbx commented Oct 26, 2018

marbx commented May 24, 2019

marbx commented May 24, 2019

marbx commented Sep 18, 2019

marbx commented Sep 19, 2019 •

edited

Loading

marbx commented Mar 11, 2020 •

edited

Loading

minion_id_remove_domain could contain string and bool values #49378

minion_id_remove_domain could contain string and bool values #49378

Conversation

marbx commented Aug 28, 2018 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Previous Behavior (example)

New Behavior (example)

Tests written?

DmitryKuzmenko left a comment

Choose a reason for hiding this comment

isbm left a comment

Choose a reason for hiding this comment

marbx commented Aug 29, 2018

terminalmage left a comment

Choose a reason for hiding this comment

isbm commented Aug 29, 2018

terminalmage commented Aug 29, 2018 • edited Loading

DmitryKuzmenko commented Aug 29, 2018

marbx commented Aug 30, 2018

isbm commented Aug 30, 2018 • edited Loading

terminalmage commented Aug 30, 2018

marbx commented Sep 1, 2018 • edited Loading

marbx commented Sep 4, 2018 • edited Loading

marbx commented Oct 4, 2018

isbm commented Oct 5, 2018

isbm commented Oct 5, 2018 • edited Loading

rallytime commented Oct 5, 2018

marbx commented Oct 5, 2018

marbx commented Oct 5, 2018

rallytime commented Oct 5, 2018

marbx commented Oct 25, 2018

gtmanfred commented Oct 25, 2018

marbx commented Oct 26, 2018

marbx commented May 24, 2019

marbx commented May 24, 2019

marbx commented Sep 18, 2019

marbx commented Sep 19, 2019 • edited Loading

marbx commented Mar 11, 2020 • edited Loading

marbx commented Aug 28, 2018 •

edited

Loading

terminalmage commented Aug 29, 2018 •

edited

Loading

isbm commented Aug 30, 2018 •

edited

Loading

marbx commented Sep 1, 2018 •

edited

Loading

marbx commented Sep 4, 2018 •

edited

Loading

isbm commented Oct 5, 2018 •

edited

Loading

marbx commented Sep 19, 2019 •

edited

Loading

marbx commented Mar 11, 2020 •

edited

Loading