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

Remove the ToOCI functions from the specs-go package #208

Merged
merged 1 commit into from
Jun 10, 2024

Conversation

elezar
Copy link
Contributor

@elezar elezar commented May 28, 2024

This change removes the ToOCI functions from the specs-go package.
They are implemented as private toOCI functions in the cdi package instead.
This means that there is no dependency on the OCI runtime spec for clients that
only need a CDI spec definition.

NOTE: This is a breaking change in that the ToOCI functions no longer exist in the specs-go module, but the impact of this should be minimal and is worth the removal of the require github.com/opencontainers/runtime-spec dependency from the spec definitions.

@elezar elezar requested review from klihub and marquiz May 28, 2024 12:37
@elezar elezar self-assigned this May 28, 2024
@elezar elezar mentioned this pull request May 28, 2024
6 tasks
klihub
klihub previously approved these changes May 28, 2024
@elezar
Copy link
Contributor Author

elezar commented May 28, 2024

/cc @pohly

limitations under the License.
*/

package cdi
Copy link
Contributor Author

@elezar elezar May 28, 2024

Choose a reason for hiding this comment

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

@klihub @marquiz since moving these functions here is a breaking change, do we want to add "empty" implementations to specs-go so that we can mark these as deprecated. Something like:

// ToOCI returns the opencontainers runtime Spec Hook for this Hook.
// 
// Deprecated: Use cdi.Hook.ToOCI instead.
func (h *Hook) ToOCI() interface{} {

}

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to keep these APIs in the specs-go and add deprecation warnings to them, so both old and new APIs would co-exist for a deprecation period?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue with that is that it would not remove the dependency, but yes, we could do that. Would you have a recommendation as to what the deprecation period should be?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess if we really deprecate this the right way, we don't get rid of the dependency yet (so we don't return an interface either as suggested). The other alternative would be to do it the 'incorrectly/agressively', and because of the suggested empty interface-return signature change I am not sure if that is what @bart0sh was after, to keep a dummy implementation that is marked as deprecated, and maybe even panic from it.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for doing it aggressively. It makes sense after the above explanation, thanks @klihub

@elezar
Copy link
Contributor Author

elezar commented May 31, 2024

@klihub @bart0sh as a general question, I was wondering whether we should actually REMOVE the ToOCI functions here instead (in practice we would make them private). My motivation is that these functions do not make sense in isolation and are only consumed from the Apply functions which actually update the spec.

If we do have clients that use these directly, we can consider how to better add these APIs.

This change removes the ToOCI functions from the specs-go package.
They are implemented as private toOCI functions in the cdi package instead.
This means that there is no dependency on the OCI runtime spec for clients that
only need a CDI spec definition.

Signed-off-by: Evan Lezar <elezar@nvidia.com>
@elezar elezar changed the title Move ToOCI functions out of specs-go package Remove the ToOCI functions from the specs-go package May 31, 2024
@elezar elezar requested review from klihub and bart0sh May 31, 2024 11:27
@elezar elezar dismissed klihub’s stale review May 31, 2024 11:27

functions made private

@klihub
Copy link
Contributor

klihub commented May 31, 2024

@klihub @bart0sh as a general question, I was wondering whether we should actually REMOVE the ToOCI functions here instead (in practice we would make them private). My motivation is that these functions do not make sense in isolation and are only consumed from the Apply functions which actually update the spec.

If we do have clients that use these directly, we can consider how to better add these APIs.

@elezar Good point. If with this change we don't have cross-package use any more then I think making them private makes sense, since/if we don't consider them useful for external users

@elezar
Copy link
Contributor Author

elezar commented May 31, 2024

@klihub I have updated the latest revision with private functions. PTAL.

Copy link
Contributor

@klihub klihub left a comment

Choose a reason for hiding this comment

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

LGTM

@bart0sh
Copy link
Contributor

bart0sh commented May 31, 2024

LGTM

@klihub klihub merged commit f33d15b into cncf-tags:main Jun 10, 2024
7 checks passed
@elezar elezar deleted the move-to-oci-functions branch June 10, 2024 11:17
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.

None yet

3 participants