[Bubballoo3]

Please review our Contributing Guide before submitting a pull request.

What does this PR do?

Adds a test that rigidly verifies the expectations laid out in the adapter inheritance model. Namely that subclasses of OodCore::Job::Adapter must present a perfect subset of the Adapter interface. So for each public method of Adapter, a subclass must either

• override that method with one that accepts exactly the same args as the base class method
• refuse to override that method, leading to the base class raising a NotImplementedError.

I am opening this first with just the tests to generate a failure I can refer to when I have fixed all the adapters. I do want to bring up one failure in particular, stemming from

ood_core/lib/ood_core/job/adapters/ccq.rb

Lines 107 to 123 in 8c5ca82

	# This adapter does not implement hold and will always raise
	# an exception.
	# @param id [#to_s] the id of the job
	# @raise [JobAdapterError] always
	# @return [void]
	def hold(_)
	raise NotImplementedError, "subclass did not define #hold"
	end
	# This adapter does not implement release and will always raise
	# an exception.
	# @param id [#to_s] the id of the job
	# @raise [JobAdapterError] always
	# @return [void]
	def release(_)
	raise NotImplementedError, "subclass did not define #release"
	end

Which attempts to raise the NotImplementedError from inside the subclass, rather than just allowing the base class to do the same. While this practice is not directly buggy like the original issue, I would argue that it is still not recommended, as it unnecessarily duplicates code and generally breaks the pattern I would expect, though I could begrudgingly be convinced to carve out an exception for this if we think its improper to remove this from the ccq adapter docs (even though the inline docs only mention that it will never be implemented).

Related issue

Closes #1039

Testing

• Tests included
• No tests needed — reason: ___

Checklist

• Follows project code style and conventions
• Documentation provided (if new feature, adapter or behavior change)
• This is a large feature and was discussed in an issue first (if applicable)

Anything else?

Screenshots, context, or anything reviewers should pay close attention to.

[Bubballoo3]

This is finally passing the test, but did end up stretching a bit past what I had originally expected. Two things are worth calling out

1. All adapters now allow a host: parameter in info_all. This was previously only defined on Systemd and LinuxHost, but not on the base class. Following from the principles laid out above, I added it to the base class and every other subclass, so that the interface is consistent. I also added a doc entry on the host: arg, which had no existing mention. Very open to feedback on that entry as it is not a param I am super familiar with

2. Systemd and LinuxHost now name unused parameters in info_where_owner. This is somewhat similar to the ccq case in the initial comment, but instead of duplicating directly from the base class, these methods ignored the user param and thus defined it as _. This is something we encourage when possible, but I'd argue is confusing in the context of the shared adapter interface. The inline docs directly above mention owner, but this param would be difficult to recognize since it isn't actually called that in the definition. However it is worth mentioning that named or unnamed args present the same way to callers, so this was never going to lead to a bug. With that, I could be convinced to allow unnamed args when they are unused, by only asserting the number and requirement status of positional args, ignoring the actual names assigned.

[johrstrom]

Doesn't a linter say this is correct?

[Bubballoo3]

Yeah I could have gone either way on this, so I will update to not check names of positional args and revert this change

[johrstrom]

Maybe check with rubocop - I'm only saying that from memory. Maybe it'll say _owner is correct, I'm not 100% sure.

[johrstrom]

The parent class doesn't have the host keyword. Not sure why it was implemented in linux_host - I can't find it being used in OnDemand.

ood_core/lib/ood_core/job/adapter.rb

Lines 56 to 58 in 8c5ca82

	def info_all(attrs: nil)
	raise NotImplementedError, "subclass did not define #info_all"
	end

If anything we can open the interface to attrs and **kwargs but it seems unnecessary.

[johrstrom]

Didn't scroll down far enough - you add it to the parent class.

In any case the comment I think still stands - open it up to **kwargs or remove it from the child clasess. I would probably vote for the latter as I don't see it actually being used in OnDemand.

[Bubballoo3]

Yeah I might have to sit with this for a bit as I'm not sure I like either route very much. It seems kinda wrong to rip the host filtering feature out of LinuxHost and Systemd altogether when it has been there for 7ish years, but if we aren't using it then maybe nobody would care. On the flip side, a blanket **kwargs feels like it adds margin for error here, when the intent of the ticket was making the adapter interface more consistent to reduce confusion caused downstream.

Looking back, it would have been nice if it had originally been added as a separate method, like info_where_host(, but this far along I bet its about the same impact to change it as it is to rip it out altogether, the impact being that older unmaintained stuff could break

[Bubballoo3]

After thinking about this a bit more I think I want to relax the initial requirements to reduce the changes somewhat. I would be happy to drop the 'Identical interface' requirement to be instead something like 'The interface defined in the base class must be operable within all subclasses' That allows us to rename args (like _ when an arg is unused) and allows individual adapters to add args, like we see with Systemd and LHA. So it reverses the more ambiguous changes seen here while still providing the primary benefit, that consumers need only refer to the base class' interface and have trust that their code will work with any subclass.