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

Allow removing keys based on source file. #152

Merged
merged 1 commit into from
Apr 9, 2019
Merged

Allow removing keys based on source file. #152

merged 1 commit into from
Apr 9, 2019

Conversation

polymeter
Copy link
Contributor

This change allows keys to be not only added, but also removed by providing a source file. This is supported by ssh_auth.absent since salt 2015.8.0. Hope I have not overlooked anything.

@alxwr
Copy link
Member

alxwr commented Mar 19, 2019

@aboe76 Do we need to keep compatibility with pre-2015.8.0?

Copy link
Member

@alxwr alxwr left a comment

Choose a reason for hiding this comment

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

Hi @polymeter ! Thanks for your PR! We'll just need to make sure this change won't break existing setups.

@@ -12,7 +12,7 @@
{%- else %}
- user: {{ identifier }}
{%- endif %}
{%- if 'present' in key and key['present'] and 'source' in key %}
{%- if 'source' in key %}
Copy link
Member

Choose a reason for hiding this comment

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

Hi @polymeter ! Thanks for your PR!
I fear this change would break existing setups. Would you please alter your PR in a way that present if false overrides the presence of source?
Please also update pillar.example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey,
wouldn't that overriding essentially make us go back to the previous behaviour? Using a combination of present=False with a given source is my very goal here.
If we need to support 2015.5 and earlier, okay - I just had not expected that, given its lifetime. However, I think we'll need to directly query the salt version and differentiate between the two cases then. Would that be okay with you?

Copy link
Member

Choose a reason for hiding this comment

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

@polymeter my idea was to go with (your) new behavior, if present is absent, but stick to the old behaviour if present: False.
This way we don't need to care about versions, and don't break existing setups.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking about something like this:

{%- if 'source' in key and not (present in key and key['present'] == False) %}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey, I don't mean to stretch this discussion unneccessarily, but I'd find that behaviour very counter-intuitive, thus requiring explicit documentation. We'd have a tristate-like option looking like a bool, and source-users could not simply switch present from True to False without breaking it.
So, I'd still vote for checking the salt version number, but I will leave the final decision to you and update the PR accordingly.

@alxwr alxwr self-assigned this Mar 19, 2019
@daks
Copy link
Member

daks commented Mar 20, 2019

The page https://s.saltstack.com/product-support-lifecycle/ lists only 2019.2 and 2018.3 as still supported, but I'm not sure if this is for commercial product or affects open source version too.

@polymeter
Copy link
Contributor Author

Hey @daks
That's what I found, too. The support cycle seems to apply to both the open source and the Enterprise version, as per https://s.saltstack.com/saltstack-enterprise-system-requirements/ (section "Supported salt versions").

@myii
Copy link
Member

myii commented Mar 25, 2019

@alxwr In the various conversations that have come up about this, it's generally agreed that the very oldest versions we need to support can be judged by the "Extended life support ends" date on https://s.saltstack.com/product-support-lifecycle/. In that case, 2016.11 is the oldest that needs to be considered and even that is winding down by May 31, 2019. There's even a view that we should only consider the "Phase 2 support ends" date and in that case, the oldest version is 2018.3, which is out of support in just over a month. Essentially, which ever way we look at it, 2015.8 is long gone, so I recommend not considering that a factor in the final decision with this PR.

As for the implementation itself, I'll defer that back to your expert opinions!

@myii
Copy link
Member

myii commented Mar 26, 2019

@alxwr Got more information from upstream: they only actively support 3 releases. So we only have to concern ourselves with back to 2017.7. See this comment for more information.

@alxwr
Copy link
Member

alxwr commented Apr 9, 2019

@myii @polymeter Thanks for your valuable input and sorry for the long pause! :-)
I was never concerned with the versions of Salt itself, but rather with existing Pillar data. Because semantic versioning has not yet been implemented as described in https://github.com/saltstack-formulas/template-formula#id3, I'm somewhat reluctant to introduce incompatibilities.

Nevertheless it's a minor change and every admin is called to maintain her own clone of the formula repo anyway. @polymeter I'll merge the PR as it is, because {%- if 'source' in key %} is so much more maintainable. Thanks for your work and your constructive criticism!

@alxwr alxwr merged commit b5ac5e0 into saltstack-formulas:master Apr 9, 2019
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