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

Rename RunPullImagesCheck to CheckAndRunPulllImages #1352

Closed
MalloZup opened this issue Jan 15, 2019 · 11 comments · Fixed by kubernetes/kubernetes#74399
Closed

Rename RunPullImagesCheck to CheckAndRunPulllImages #1352

MalloZup opened this issue Jan 15, 2019 · 11 comments · Fixed by kubernetes/kubernetes#74399
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. priority/backlog Higher priority than priority/awaiting-more-evidence.
Milestone

Comments

@MalloZup
Copy link

MalloZup commented Jan 15, 2019

This issue is a follow-up from kubernetes/kubernetes#72870

i will tackle this once that pr is merged.

Problem was that the name of function can lead confusion. We should make more explicit that we also pulluing image really and not run only checks as the name could suggest

@MalloZup
Copy link
Author

/assign MalloZup

@MalloZup
Copy link
Author

if you have another name suggestion feel free to suggest

@MalloZup MalloZup changed the title Rename RunPullImagesCheck to CheckAndPullImages Rename RunPullImagesCheck to CheckAndRunPulllImages Jan 15, 2019
@RA489
Copy link
Contributor

RA489 commented Jan 16, 2019

@MalloZup I can look into it.

@neolit123
Copy link
Member

@MalloZup @RA489
i'm not a big fan of this rename.
we can technically look for small renames like this all over the place, but instead we should target higher priority work.

cc @rosti
to weight in if should do this rename.

@neolit123 neolit123 added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. labels Jan 16, 2019
@MalloZup
Copy link
Author

MalloZup commented Jan 16, 2019

@neolit123 we can sync in slack for issue prios.o me I felt that it was a follow-up for later. I agree it isn't high pro

@MalloZup
Copy link
Author

@rosti for me should be valid to rename it because isn't much implicit by the name that we are also pulling the images.

However if we rename this function, also the other name of function might be in the same situation. I had just a look. Anyways, i'm ok with both decision, to rename or not. tia 🚀

@RA489
Copy link
Contributor

RA489 commented Jan 16, 2019

@neolit123 sure, please assign some high priority items for me in todays kubeadm office hours.

@rosti
Copy link

rosti commented Jan 16, 2019

@neolit123 my view on this one is that the rename is necessary. The func is named that way for historical reasons that are no longer true. Hence the change is necessary, but not at all urgent.
Therefore, this has a backlog priority to me.
It's a good issue for novice contributors.

@neolit123
Copy link
Member

/priority backlog

@k8s-ci-robot k8s-ci-robot added the priority/backlog Higher priority than priority/awaiting-more-evidence. label Jan 16, 2019
@neolit123
Copy link
Member

@RA489
we will be mostly busy with other items today, but please take a look at any tickets labeled, bug, help-wanted priority-important-*.

@timothysc timothysc added this to the Next milestone Jan 25, 2019
@rosti
Copy link

rosti commented Feb 19, 2019

Ok, this is a bit more complex than just a rename:

  1. We need to unify the code, that is serving under "pull pre-flight check" and "kubeadm config images pull".
  2. Certainly, what we are executing as a "pre-flight check" is an action and although we do it at pre-flight check time it is not such. Therefore it's best for it to not take the pre-flight check implementation model and not live in a file with the rest of the checks. It should be a single synchronous func, that simply pulls images from a list and CRI runtime.
  3. These are more precisely called "ControlPlane" rather than "All" images. Hence having name as PullControlPlaneImages and s/GetAllImages/GetControlPlaneImages is more accurate.

@RA489 I'll be happy to do a review of a PR about this, but this is a backlog and "not current milestone" issue, also it's not a very trivial to do, so I'll recommend jumping off to something more important in the current milestone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. priority/backlog Higher priority than priority/awaiting-more-evidence.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants