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

Update copy image logic to match create image. #828

Closed
wants to merge 2 commits into
base: trunk
from

Conversation

Projects
None yet
4 participants
@illfelder
Contributor

illfelder commented Jun 30, 2016

Image "guestOsFeatures" should behave the same during image copy and creation.

Description

  • Unify "guestOsFeatures" logic in image copy and create.
  • Moved image copy next to image create because they are highly related.

Status

done, ready for review

Checklist (tick everything that applies)

  • Code linting (required, can be done after the PR checks)
  • Documentation
  • Tests
  • ICLA (required for bigger changes)
@erjohnso

This comment has been minimized.

Show comment
Hide comment
@erjohnso
Member

erjohnso commented Jun 30, 2016

/cc @supertom

@illfelder

This comment has been minimized.

Show comment
Hide comment
@illfelder
Contributor

illfelder commented Jul 6, 2016

@illfelder

This comment has been minimized.

Show comment
Hide comment
@illfelder

illfelder Jul 7, 2016

Contributor

Friendly ping?

Contributor

illfelder commented Jul 7, 2016

Friendly ping?

@supertom

This comment has been minimized.

Show comment
Hide comment
@supertom

supertom Jul 7, 2016

Contributor

Sorry for the delay. Looks fine and tests/lint check pass.

We've now duplicated that guest_os_feature check in both the create and copy methods - what do you think about creating a helper function to reduce the duplication (any chance of any other API exposing this somehow)? Also, are there more guest_os_features coming? Should we make a static list?

Contributor

supertom commented Jul 7, 2016

Sorry for the delay. Looks fine and tests/lint check pass.

We've now duplicated that guest_os_feature check in both the create and copy methods - what do you think about creating a helper function to reduce the duplication (any chance of any other API exposing this somehow)? Also, are there more guest_os_features coming? Should we make a static list?

@illfelder

This comment has been minimized.

Show comment
Hide comment
@illfelder

illfelder Jul 7, 2016

Contributor

I moved the functions together for that purpose and considered writing a helper function. I decided against it for two reasons.

  1. I think the logic for the particular feature is limited to those two functions and will not need to be used elsewhere.
  2. The two functions where the logic is duplicated (image create and copy) are serving the same purpose and should be combined. I didn't want to deal with combining the functions for this change which is why I left the logic duplicated for now.

We will want to add more guest_os_features in the near future, so it should be a static list. Any suggestions for where I should put that? Define a global variable in the file?

Contributor

illfelder commented Jul 7, 2016

I moved the functions together for that purpose and considered writing a helper function. I decided against it for two reasons.

  1. I think the logic for the particular feature is limited to those two functions and will not need to be used elsewhere.
  2. The two functions where the logic is duplicated (image create and copy) are serving the same purpose and should be combined. I didn't want to deal with combining the functions for this change which is why I left the logic duplicated for now.

We will want to add more guest_os_features in the near future, so it should be a static list. Any suggestions for where I should put that? Define a global variable in the file?

@illfelder

This comment has been minimized.

Show comment
Hide comment
@illfelder

illfelder Jul 8, 2016

Contributor

Updated to make the feature a static list.

Contributor

illfelder commented Jul 8, 2016

Updated to make the feature a static list.

@supertom

This comment has been minimized.

Show comment
Hide comment
@supertom

supertom Jul 8, 2016

Contributor

Thanks for adding the list of OS features. I agree, the functions should be combined, but we can deal with that later on. LGTM.

Contributor

supertom commented Jul 8, 2016

Thanks for adding the list of OS features. I agree, the functions should be combined, but we can deal with that later on. LGTM.

},
}
if guest_os_features:

This comment has been minimized.

@Kami

Kami Jul 10, 2016

Member

It looks like the same logic is duplicated in ex_create_image method.

It would be great if it can be refactored in a shared utility method to avoid duplication and make maintenance easier.

@Kami

Kami Jul 10, 2016

Member

It looks like the same logic is duplicated in ex_create_image method.

It would be great if it can be refactored in a shared utility method to avoid duplication and make maintenance easier.

This comment has been minimized.

@illfelder

illfelder Jul 10, 2016

Contributor

I agree - if you take a look at the conversation thread for the pull request, we discuss exactly that change.

@illfelder

illfelder Jul 10, 2016

Contributor

I agree - if you take a look at the conversation thread for the pull request, we discuss exactly that change.

This comment has been minimized.

@illfelder

illfelder Jul 12, 2016

Contributor

The TL;DR from the conversation was that the duplicated functionality I add will only be used by those two functions, so it doesn't make sense to move it into a helper. The two functions, however, should be combined since they do (essentially) the same thing. That should be done as a separate pull request. Do you have any other concerns before merging?

@illfelder

illfelder Jul 12, 2016

Contributor

The TL;DR from the conversation was that the duplicated functionality I add will only be used by those two functions, so it doesn't make sense to move it into a helper. The two functions, however, should be combined since they do (essentially) the same thing. That should be done as a separate pull request. Do you have any other concerns before merging?

@illfelder

This comment has been minimized.

Show comment
Hide comment
@illfelder

illfelder Jul 11, 2016

Contributor

Any concerns about merging?

Contributor

illfelder commented Jul 11, 2016

Any concerns about merging?

@asfgit asfgit closed this in a47a51e Jul 15, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment