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

Support GenericResources in ServiceSpec.TaskTemplate.Resources.Reservations #2327

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

johnflavin-fw
Copy link

Closes #2320

@johnflavin-fw johnflavin-fw requested a review from a team as a code owner May 14, 2024 20:48
public T getValue() {
return value;
}
public abstract T getValue();

Choose a reason for hiding this comment

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

Note to any potential reviewers: This is causing the build to fail because it is an API change; the method used to be concrete but is now abstract. But IMO it's a valid change. It isn't going to impact any users because no one was constructing these GenericResources before anyway. There was no way to do so.

I suppose I could leave it non-abstract, keep a T value property here, and make the child classes basically empty shells with declarations only. But it really doesn't make any sense for users to be able to construct GenericResource instances, this should be abstract, if you tried to pass one to the docker API it would fail.

All of that to say, I don't know how to bypass the API change check so that the build can succeed.

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.

No GenericResources in ServiceSpec.TaskTemplate.Resources.Reservations
2 participants