Skip to content

Conversation

@midnightercz
Copy link
Member

Repository.sync() operation is now supported for Yum,File and
ContainerImage Repository

@midnightercz midnightercz force-pushed the sync-support branch 4 times, most recently from f7539ad to 2b01e82 Compare February 12, 2020 17:54
Repository.sync() operation is now supported for Yum,File and
ContainerImage Repository
Copy link
Contributor

@rohanpm rohanpm left a comment

Choose a reason for hiding this comment

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

Most of my comments are about the SyncOptions classes. I think that you've added an option for pretty much everything which exists in Pulp, but not all of them make sense.

I put specific comments on some of them, and in general could you please consider cutting those down a bit into only those where there's a reasonable expectation of actually using them in our toolchain?

return f_map(response, error_fn=map_404_to_none)

def _do_sync(self, repo_id, sync_options):
if not sync_options["feed"]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have this check actually?

  1. Like any other options, it's possible for feed to be stored on the importer config itself, in which case it's not necessary to pass it. Ideally the library doesn't block this scenario.
  2. If the Pulp server really needs a feed (because it's missing from importer config), it should already be returning a meaningful error message, so we shouldn't have to duplicate the same error check here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I wanted to do this "user-friendly" and fails asap if there's obvious error. As you said feed can be used from importer but to do that I would have to implement the model and another support code, which is something I'm not that much willing to do now due lots of changes needed in another projects right now. So I thought we still override feed everytime so let's check that feed is not empty here and in the future we can implement importer model in repository model.
If workflow is: get repository model, run sync, then at that time you will have already information about importer configuration and you can decide if there's feed or not and therefore if "external" feed is required.
Another reason is log message few lines bellow this one which tells you: Syncing repository with feed: xxx.
But maybe it's unnecessary complicated and I should rather let pulp return error here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't mean that this library needs to fetch the importer and use it to initialize feed - I meant that you can just do a sync request to Pulp without including any feed, and it'll work if the importer already has a feed or it'll return a meaningful error if it doesn't. So you wouldn't have to add Importer models yet.

In any case though, I don't consider this a blocker for the merge request, seeing as it can be easily changed later without breaking compatibility.

midnightercz and others added 12 commits February 13, 2020 08:23
Co-Authored-By: Rohan McGovern <rohan@mcgovern.id.au>
Co-Authored-By: Rohan McGovern <rohan@mcgovern.id.au>
Co-Authored-By: Rohan McGovern <rohan@mcgovern.id.au>
Co-Authored-By: Rohan McGovern <rohan@mcgovern.id.au>
Co-Authored-By: Rohan McGovern <rohan@mcgovern.id.au>
Co-Authored-By: Rohan McGovern <rohan@mcgovern.id.au>
Co-Authored-By: Rohan McGovern <rohan@mcgovern.id.au>
Co-Authored-By: Rohan McGovern <rohan@mcgovern.id.au>
Co-Authored-By: Rohan McGovern <rohan@mcgovern.id.au>
- Fixed documentation for SyncOptions attributes
- Fixed wrong attribute types
- Renamed test_sync to test_sync_no_feed
- Fixed doc strings for tests
@midnightercz
Copy link
Member Author

Most of my comments are about the SyncOptions classes. I think that you've added an option for pretty much everything which exists in Pulp, but not all of them make sense.

I agree we don't use them in our toolchain - yet. This is I think third time I decided to use pubtools-pulplib for implementation of some new feature in our toolchain and I found out that there's missing functionality I need in this library. So I thought it would be better to rather implement everything that is supported then in the future find out you need to add something again.

- Make ContainerSyncOptions, FileSyncOptions, YumSyncOptions public
@midnightercz midnightercz requested a review from rohanpm February 14, 2020 14:51
midnightercz and others added 4 commits February 18, 2020 13:24
Co-Authored-By: Rohan McGovern <rohan@mcgovern.id.au>
Co-Authored-By: Rohan McGovern <rohan@mcgovern.id.au>
Co-Authored-By: Rohan McGovern <rohan@mcgovern.id.au>
Co-Authored-By: Rohan McGovern <rohan@mcgovern.id.au>
@midnightercz midnightercz requested a review from rohanpm February 18, 2020 12:26
@rohanpm rohanpm merged commit ecb3863 into release-engineering:master Feb 19, 2020
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.

2 participants