Kubeadm: Rename RunPullImagesCheck to CheckAndRunPulllImages

Created on 15 Jan 2019  路  11Comments  路  Source: kubernetes/kubeadm

This issue is a follow-up from https://github.com/kubernetes/kubernetes/pull/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

kincleanup prioritawaiting-more-evidence prioritbacklog

Most helpful comment

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.

All 11 comments

/assign MalloZup

if you have another name suggestion feel free to suggest

@MalloZup I can look into it.

@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 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

@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 :rocket:

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

@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.

/priority backlog

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

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.

Was this page helpful?
0 / 5 - 0 ratings