add Client.write_data method

[M0NsTeRRR]

If you are here to understand the changes as a user, please see:

• Breaking changes coming to Client.write method #1034

---

Hello,

This PR fix #133.

I suggest this implementation (name can be changed but i've avoided write_v2 to not lead confusion with kv v1 and v2) to handle backward compatibility.

V2 : We add this function
V3 : We replace the old one and delete this function
or
V2 : We add this function
V3 : We replace the old one and redirect this function (to keep both for a certain time, for example until v4)
VX: We remove this function

Regards,

[codecov]

Codecov Report

Merging #1028 (b0eae82) into main (b52ed19) will increase coverage by 0.21%.
Report is 2 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1028      +/-   ##
==========================================
+ Coverage   84.78%   84.99%   +0.21%     
==========================================
  Files          65       65              
  Lines        3083     3133      +50     
==========================================
+ Hits         2614     2663      +49     
- Misses        469      470       +1     

Files Changed	Coverage Δ
hvac/v1/__init__.py	88.67% <100.00%> (+0.14%)	⬆️

... and 4 files with indirect coverage changes

[briantist]

Hi @M0NsTeRRR welcome! Thanks very much for submitting this.

I like the idea of adding a new function to avoid the issue described in #133 (comment) where we would lose access to a second named kwarg without a transition period.

I think I would like to modify the idea and schedule as follows:

• v1.2.0 -- add the new function (let's call it write_data) [THIS PR]
• v2.0.0 -- update write as follows:
  • remove the (explicit) arguments path and wrap_ttl, add *args
  • set value of path and wrap_ttl based on *args (for positional) and **kwargs
  • if path or wrap_ttl values were gotten from **kwargs, they won't be set as data (this preserves previous behavior), and we'll issue a warning suggesting the use of write_data or setting these positionally
  • if data exists in kwargs we'll also warn about its impending use (see next items) and suggest write_data
• v3.0.0 -- "activate" a data parameter in write:
  • if kwargs contains data and no other non-argument keys, then this call will operate the same as write_data (forward compatible)
  • if kwargs contains both data and other data keys, raise an exception
  • otherwise, it will operate the same as write does now (backward compatible), with a deprecation warning that kwargs is to be removed in v4.0.0, suggesting using the data dictionary going forward
• v4.0.0 -- update write to its Final Form™:
  • remove *args and **kwargs, and return to defined arguments for path, wrap_ttl, and data
  • no more warnings
  • at this point, write basically becomes write_data
• v4.0.0 -- mark write_data as an alias for write in documentation
  • deprecate its name in favor of write, but because some folks may have migrated to this name, and because it has almost no maintenance burden, let's make it a long deprecation, to be removed in say, v6 or v7? Exact version can be determined later.

---

So, here's my reasoning for the more complex replacement:

• I would like to avoid a situation where people have to migrate write calls to write_data calls, and then migrate back again in a short time
  • this can be really annoying for projects using this library that want to support a range of versions
• I would like to avoid warnings that don't have a (good) way to make them go away

So the idea is that in v1.2.0 we introduce the new implementation as a new function name, so it's opt-in. write doesn't change at all: if you're hitting a use case where you need to use write_data, it's there for you.

In v2.0.0, we change the write implementation so that we can detect if our parameters were passed positionally or by keyword. If they were set positionally, we do nothing, write operates identically, and we avoid warnings.

If the arguments were passed by keyword, write still does the same operation that it used to (those values cannot be part of the data written) but a warning is issued to suggest alternatives:

1. Set path and/or wrap_ttl positionally (you can keep using the write function, but make the warning go away) -- this is decent if you know that you'll never need to be setting those names as actual data
2. Use write_data if those names might need to be part of data (explicitly, or if you're writing user-/run-time- supplied data) -- eventually down the line you'll have to change back to write
   (or, the warning can be swallowed/ignored)
   In this version, we'll also warn if kwargs contains a data key since behavior of that will change in the next version: suggest switching to write_data to avoid breakage.

In v3.0.0, we'll add a way to use a data parameter in write. This will be an actual breaking change for any call that tried to write a data key, but at this point, write continues to work the same for anyone who wasn't using a data key before
Adding this allows a caller who wanted to stick with the write function name to update their implementation to use the data dictionary instead, which will let them be forward compatible with v4.0.0, or allows a caller who changed to write_data to change back now ahead of write_data's eventual removal.

In v4.0.0, write becomes write_data, setting via kwargs no longer works, and we can keep the write_data name around as an alias for several versions to give a lot of time for folks to migrate back to write.

---

I will try to write a "guide" for callers on what change(s) to make, in what version(s), depending on their needs.

---

Sorry that was quite long! What do you think?

[M0NsTeRRR]

Hello,

No worries :)

I find the plan a bit complicated and it adds a lot of maintenance on several versions. If we follow the semantic version, a major version update can break changes. We also have to think that we will have to explain this change in the changelog and that it must remain quite readable.
It seems complex to me to project ourselves so much into the future with the end of this migration to V4 there may be time to see a complete rewrite.
Also the changes to the function itself seem rather minimal to me for an user of the library.

I would suggest :

• v1.2.0 -- add the new function (let's call it write_data) [THIS PR]
• v2.0.0 -- delete write_data and add the logic in write

Regards,

[briantist]

If we follow the semantic version, a major version update can break changes.

While it's true that we can make a breaking change more quickly than I laid out and still conform to semver, strict adherence isn't the only concern.

• Giving advance notice where possible helps users prepare for changes.
• Using warnings to do so increases the chances of changes being brought to the attention of users.
• Limiting the scope of warnings to actual calls affected helps reduce warning spam and warning fatigue.

In this case, due to the unique situation of us being unable to add the new parameter the original function first due to exacerbating the effect, we have a new function that will eventually replace the original, and this means some users will have to change the function they use twice.

I want to minimize the number of users who have to make that change and make the path as smooth as possible for everyone.

---

We also have to think that we will have to explain this change in the changelog and that it must remain quite readable.

I agree; but our changelog at the moment only consists of PR titles and links, so any explanation for these changes need to go into some PR or issue.

I will write a more coherent guide for what changes to make and when they need to be made (from the POV of a user and/or a dependent project maintainer) to help with that.

Writing from this POV should be simple than the explanations above which are all about implementation.

It seems complex to me to project ourselves so much into the future with the end of this migration to V4 there may be time to see a complete rewrite.

I can definitely understand that it seems like v4 is incredibly far in the future. It look a very long time before v1 was released, and since then, we're quite behind schedule on v2.

This is something I'm actively looking to change. I want us releasing more quickly, with fewer changes per release (mostly these will be minor releases).

But we've also committed to:

1. Remove support for Python versions in hvac when they stop being supported
2. Only remove support for a Python version in a major version of hvac

This means we should be releasing a major version a minimum of once a year, and that assumes no other breaking changes outside that schedule. Realistically, I expect we'll do ~2 major releases a year, as long as there is something breaking to release.

I also expect to release v2.0.0 very shortly after v1.2.0 (like within a few weeks at most), and if nothing else breaking is introduced except Python EoLs, that puts v3 around a year from now, and v4 around 2 years from now, not terribly long, but we can still move up some of the changes, depending on how the warnings and deprecations go, based on feedback, etc.

I can say with some confidence, that even looking 2 years out, the chance of a complete rewrite is effectively zero. If that were to happen, it's unlikely it would be with the current set of maintainers, and it would likely be a new project altogether.

Also the changes to the function itself seem rather minimal to me for an user of the library.

They are minimal! Especially if your scope is that you have a script or small project, or an application where you can strictly control your versions.

If you maintain a project that also needs to support a range of library versions (such as one of the projects I maintain), then more overlap is appreciated :)

---

I find the plan a bit complicated and it adds a lot of maintenance on several versions
I think I can also see that the plan sounds complicated when explained all at once.

I think the maintenance needed per version is quite minimal, and can be planned out in advance (which I will write up specific issues for).

I also want to stress that the responsibility for that maintenance is on us maintainers. While this PR and any future contributions are very much appreciated, I do not expect that this PR means you must also implement all the other steps.

The warning stuff in particular is probably not very fun to do, but I'm prepared to implement it for v2.0.0 because I want to get that release out quickly. And I feel the same about the other milestones.

---

We can continue this discussion (and probably we should move it back to #133), but I think we both at least agree that this PR can be added to v1.2.0 no matter what, so we do not have to finish the decision before moving forward with this. 🙂

Thanks again for your time and this contribution.

[M0NsTeRRR]

Ok I understand your point of view, it's understable. One of my concern was of course the release of v1 who tooks 5 years but as you are the author of the projet you have the clearest view of it.

Your welcome :)

[briantist]

This looks great, thank you! I'm going to merge soon, I want to write up the other issues for future changes, and to provide guidance for users before merging. Very excited to get this released!

[M0NsTeRRR]

Sure your welcome, I will fix ansible issue once you release a tag. Will you tag a new release after this PR or I have to wait a bit ?

[briantist]

Sure your welcome, I will fix ansible issue once you release a tag. Will you tag a new release after this PR or I have to wait a bit ?

As it happens, this is the last remaining PR that we're planning for v1.2.0 so we'll be doing a new release pretty quickly after merging this one.

Let's chat on the community.hashi_vault issue as well. Unless we make the decision to raise the minimum supported hvac, we're going to need to support the earlier version of hvac for a bit on the Ansible collection, and so we need a bit of code to determine which method to use, and tests around that. But I'm happy to help with some of the implementation there too!