[#698] move definition of IRODS_VERSION

[d-w-moore]

No description provided.

[alanking]

Sorry, had a thought while I was staring at this... The way it is now is actually fine since the value is not changing. I just wanted to raise a point for consideration.

[alanking]

# it

See @trel's comment below.

[trel]

should we add these words (or similar) in comment form somewhere? for the next reader?

should we add these words (or similar) in comment form somewhere? for the next reader?

Hmm... fair enough. Yes, let's put a comment above the new/moved declaration.

[korydraughn]

From what I can tell, the presented changes ...

• Maintain backward compatibility
• Are for testing purposes only

Is that correct?

Do we have a mechanism for minimum compatibility?
Do we document a minimum compatibility version in the README?

[d-w-moore]

From what I can tell, the presented changes ...

• Maintain backward compatibility
• Are for testing purposes only

Is that correct?

Do we have a mechanism for minimum compatibility? Do we document a minimum compatibility version in the README?

That's correct, it's for testing only as far as we use it. We don't have formal mechanisms for compatibility other than the limits coded into the test suite

[trel]

should the name convey that this is for testing only?

[korydraughn]

Got it. Like @trel said, let's capture that in a comment for future readers.

Let's also document the minimum compatibility version somewhere too, maybe near the top of the README so users always know what the library targets.

[korydraughn]

should the name convey that this is for testing only?

Looking at @d-w-moore's explanation again, I'm now wondering why testing is blocked when targeting a more recent iRODS server?

Wouldn't that make testing more difficult? i.e. You'd have to bump the compatible version or relax the compatibility check. This is already covered by the following code.

python-irodsclient/irods/helpers/__init__.py

Lines 57 to 65 in 94b8594

	if test_server_version:
	connected_version = _get_server_version_for_test(session, curtail_length=3)
	advertised_version = IRODS_VERSION[:3]
	if connected_version > advertised_version:
	msg = (
	"Connected server is {connected_version}, "
	"but this python-irodsclient advertises compatibility up to {advertised_version}."
	).format(**locals())
	raise iRODS_Server_Too_Recent_For_Testing(msg)

Couldn't the tests and PRC just adjust its implementation based on the server version? That is what other iRODS projects do. The only thing that would block that approach is if it required breaking changes to the library's API.

[d-w-moore]

should the name convey that this is for testing only?

Looking at @d-w-moore's explanation again, I'm now wondering why testing is blocked when targeting a more recent iRODS server?

Wouldn't that make testing more difficult? i.e. You'd have to bump the compatible version or relax the compatibility check. This is already covered by the following code.

Maybe. My reasoning in doing it this way wasn't to make testing easier, but to remind us - automatically, based on messages issuing from the test run - that we're attempting a flawed test because the server is too recent a version for the PRC's advertised compatibility. This may be more an issue for when users attempt a test run at the bench; that is, outside the now normal Github action, in which we are always installing and against a specific iRODS server version. In which case, you would hope, someone would have remembered to update the IRODS_VERSION constant to reflect the new compatibility level.

But my point is, with things the way they are, if you haven't done that update,the test framework makes sure you are reminded to do so, in no uncertain language.

I don't mind changing it to implement new priorities, but I have no dog in the fight.

[d-w-moore]

On the subject of whether IRODS_VERSION is (or more accurately, whether it was originally intended) for testing only I don't know. It's sent to the server in the initial StartupPack message in setting up a connection. It was already there when I started maintaining PRC. I used it for deciding when a server "was too recent for testing" but perhaps there were other imagined uses for it -- ?

[korydraughn]

As you said, it's primarily used for testing. It's fine for now.

[d-w-moore]

Sorry, had a thought while I was staring at this... The way it is now is actually fine since the value is not changing. I just wanted to raise a point for consideration.

I wanted to address this comment , especially since it's been suggested we change the README to explain the significance of IRODS_VERSION....

Since the "way it is now" could either refer to (a) leaving present commits "as-is", or (b) simply keeping the definition of IRODS_VERSION identifier unchanged in name and location (ie, remove the present commit, so that this PR does not change any code, only the README )....

Let's seriously consider (b) - that is, changing this whole thing to a documentation exercise. I.e. maybe it's ok for a testing-only thing to remain two namespaces deep.

For me, the concern over changing it is motivated by the impossibility of moving a definition from one namespace (presently irods.message) to another (irods) without potentially introducing breaking changes. Liabilities exist because Python, unlike C++, can only duplicate things ie there are no true "references" in cross-module importation of scalar values, only duplications. Also, nothing is truly constant; so, if module m2 has imported X from m1, any other module could subsequently, maliciously, execute m2.X = "hello!" without an error arising, and we'd have two different values for "constant" X depending on where you view it from.

[korydraughn]

On the thought of a malicious module changing the value, isn't that true of all python code? AFAIK everything in python is mutable, which means people who care about that kind of thing have to know what they are running.

If a consumer of the library changes the value, that's on them because the value is meant to be treated as read-only.

So my question is ... is the presented code change introducing anything that wasn't already true of the implementation?

[d-w-moore]

On the thought of a malicious module changing the value, isn't that true of all python code? AFAIK everything in python is mutable, which means people who care about that kind of thing have to know what they are running.

If a consumer of the library changes the value, that's on them because the value is meant to be treated as read-only.

So my question is ... is the presented code change introducing anything that wasn't already true of the implementation?

No, it isn't right now presenting anything new or untrue. I subscribe to your view as did - evidently - Mr. Van Rossum and company. Yeah I wasn't so worried about the malicious part, not really - and certainly am happy enough to keep things as they are.

[d-w-moore]

should the name convey that this is for testing only?

Looking at @d-w-moore's explanation again, I'm now wondering why testing is blocked when targeting a more recent iRODS server?

Wouldn't that make testing more difficult? i.e. You'd have to bump the compatible version or relax the compatibility check. This is already covered by the following code.

python-irodsclient/irods/helpers/__init__.py

Lines 57 to 65 in 94b8594

	if test_server_version:
	connected_version = _get_server_version_for_test(session, curtail_length=3)
	advertised_version = IRODS_VERSION[:3]
	if connected_version > advertised_version:
	msg = (
	"Connected server is {connected_version}, "
	"but this python-irodsclient advertises compatibility up to {advertised_version}."
	).format(**locals())
	raise iRODS_Server_Too_Recent_For_Testing(msg)

Couldn't the tests and PRC just adjust its implementation based on the server version? That is what other iRODS projects do. The only thing that would block that approach is if it required breaking changes to the library's API.

Back to the subject of blocking testing vs implementations that change based on server version.

Of the latter option, I believe we already do a quite a bit, and it's quite visible library-wide if you grep for "server_version".

I believe the motivating factor for blocking testing for "too new a server" is that historically tests failed whenever the server modernized (unexpectedly, for me at least and probably for users at large if they dare to run the tests). Accordingly, this tentative "block" on running the suite was then partly a way to inform the person testing on the command line that if they ran the whole suite or a large section thereof (ie meta_test.py), they should not expect just out-of-hand for all tests thus run to " just pass."

If someone can argue that point down, I'll gladly change irods.test.helpers version of make_session to go the same way as the irods.helpers version, ie with the test_server_version option being False by default.

[korydraughn]

That's fine for now. Letting it ride.

@d-w-moore Sounds like this is ready now?