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

Simplify the name update call stack #1385

Merged
merged 1 commit into from
Oct 14, 2022
Merged

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Oct 13, 2022

Instead of going

  • 3 store methods
  • store.updateNames+enum
  • 3 sub-store methods
  • subStore.updateNames+enum
  • applyNameOperation+enum,

simplify to

  • 3 store methods
  • store.updateNames+enum
  • subStore.updateNames+enum
  • applyNameOperation+enum,

Should not change behavior. Looking purely at updateNameOperation, invalid values would now be detected after doing more work, but there is no way for an external caller to trigger use of an invalid value anyway.

@rhatdan
Copy link
Member

rhatdan commented Oct 13, 2022

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM, needs a rebase @mtrmac

Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

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

RemoveNames and AddNames are not deprecated instead they were added only to deprecate racy SetNames. Here: #1153

Downstream is still using RemoveNames or AddNames and this breaks public API without any deprecation notice IMHO.

Last switch in one of the downstreams happend here containers/buildah#4190 but there are many more PR's.

Edit: It seems most of the invocation is using Store's API but I am not sure if any invocation is directly using Container,Image or Layers's API

@nalind
Copy link
Member

nalind commented Oct 14, 2022

I'm fine with dropping the methods from rwLayerStore/rwImageStore/rwContainerStore - those are package-private types. The exported interfaces don't change, so LGTM.

@flouthoc
Copy link
Collaborator

I'm fine with dropping the methods from rwLayerStore/rwImageStore/rwContainerStore - those are package-private types. The exported interfaces don't change, so LGTM.

Ah i see, SGTM as well since I see downstreams only using Store directly which is still public so that clears my doubt. LGTM

@mtrmac There is a conflict in the PR though, could you please rebase.

Instead of going
	3 store methods
	store.updateNames+enum
	3 sub-store methods
	subStore.updateNames+enum
	applyNameOperation+enum,
simplify to
	3 store methods
	store.updateNames+enum
	subStore.updateNames+enum
	applyNameOperation+enum,

Should not change behavior. Looking purely at updateNameOperation,
invalid values would now be detected after doing more work,
but there is no way for an external caller to trigger use of
an invalid value anyway.

Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac
Copy link
Collaborator Author

mtrmac commented Oct 14, 2022

Rebased, tests pass.

@rhatdan rhatdan merged commit 910667d into containers:main Oct 14, 2022
@mtrmac mtrmac deleted the update-names branch October 17, 2022 14:26
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.

5 participants