Create architectures and components when creating a release

adrianabedon commented 5 months ago

This PR adds the following:

Allows for a single api call to create release, components, and architectures.
Adds retrieve functionality to the release creation endpoint. If a release already exists, then the existing release will be retrieved.
The release creation endpoint did not originally return a task, which is a bug since it can add content to a repository. This PR updates the endpoint to return a task.

closes #1038

daviddavis commented 5 months ago

The docs test failure is from this script in the docs which is expecting the release endpoint to return a release object instead of a task:

https://github.com/pulp/pulp_deb/blob/3a0ef81f6cbf876abdaf53559a3d96c7010485b9/docs/_scripts/structured_repo.sh#L34

This script is on this page:

https://docs.pulpproject.org/pulp_deb/workflows/upload.html#create-a-structured-repo-manually

daviddavis commented 5 months ago

With your change, these lines shouldn't even be needed anymore:

https://github.com/pulp/pulp_deb/blob/3a0ef81f6cbf876abdaf53559a3d96c7010485b9/docs/_scripts/structured_repo.sh#L37C1-L39C1

quba42 commented 5 months ago

@daviddavis So, since we are adding retrieve functionality (changing API behavior), if we were to adhere to semantic versioning, then we would need to call the next release after merging this 4.0.0, right?

Maybe I have wrong associations regarding X version bumps, but this feels wrong to me. I feel like an X version bump also creates the expectation that this is some kind of major release with many new features (rather than one small feature that happens to change some API behavior).

@mdellweg Do you have an opinion or intuition on this?

quba42 commented 5 months ago

Regarding the failed docs test, this should turn green once https://github.com/pulp/pulp_deb/pull/1074 is merged (and this PR rebased on top of it).

mdellweg commented 5 months ago

@daviddavis So, since we are adding retrieve functionality (changing API behavior), if we were to adhere to semantic versioning, then we would need to call the next release after merging this 4.0.0, right?

Maybe I have wrong associations regarding X version bumps, but this feels wrong to me. I feel like an X version bump also creates the expectation that this is some kind of major release with many new features (rather than one small feature that happens to change some API behavior).

@mdellweg Do you have an opinion or intuition on this?

I'm not an expert here. To some extend every change you do (even just bugfixes) are changing some behaviour. That's why we call it software. Because it is not completely rigid. Even "just" adding a new endpoint is normally not considered a breaking change, but if you had someone relying on /api/foo/ returning 404 you'd break them. I think it's really hard to tell where to draw the line. In particular, adding the retrieve functionality is like stopping to return an error condition and actually doing what the user intended the server to do. Is this a breaking change? I'd say no.

mdellweg commented 5 months ago

Having said all that, semver is not about "big changes", but only about "did you break a user?". OTOH I believe semver is designed for shared libraries. Applying it to a REST API is already quite a stretch.

daviddavis commented 5 months ago

I think in Pulp we've always allowed bug fixes in Y/Z releases to break backwards compatibility and I think the part of this change that has the API return a task instead of a release is actually a bug fix since the code should not have added content to a repo without dispatching a task. Maybe this logic extends to the retrieve functionality too I suppose. I'd suggest adding a breaking changelog entry here though for visibility.

The semver docs say that it applies to public APIs and I think REST APIs fall under that category. The CLIs I have worked on (hammer, pulp-cli, and our internal one) have also tried to abide by semver too and they are not shared libraries. Moreover, my reading of semver is that it permits the defining of new features/endpoints in Y releases and it doesn't cover the cases where users are relying on missing or erroneous functionality.

I agree that semver is about communicating breaking changes. I think the best way to convey big new features would be to use the changelog not the version number.

adrianabedon commented 5 months ago

I went ahead and added a test case to the PR.

quba42 commented 4 months ago

I just tested using something like the following workflow:

NAME='empty-metadata-repo'
REPO_HREF=$(pulp deb repository create --name=${NAME} | jq -r '.pulp_href')

RELEASE_OPTIONS=(
  distribution=custom
  codename=custom
  suite=stable
  version=1
  origin=myorigin
  label=mylabel
  description=mydescription
)

http ${PULP_URL}/pulp/api/v3/content/deb/releases/ repository=${REPO_HREF} architectures:='["amd64", "i386"]' components:='["main", "contrib", "non-free"]' "${RELEASE_OPTIONS[@]}"

pulp deb publication create --repository=${NAME}
pulp deb distribution create --name=${NAME} --base-path=${NAME} --repository=${NAME}

I then went and destroyed the repo again and re-added the release with only some of the components and architectures, adding the rest in a separate second API call. I noticed the following slightly odd behavior:

The Release will always be listed in the created_resources of the task (regardless of whether it already exists in Pulp or not).
Any relevant ReleaseComponents and ReleaseArchitectures will be listed in the created_resources of the task only if they were actually created by the API call. If they already existed within Pulp and were retrieved, they are still added to any resultant new repository version, but they are not listed in created_resources.

I believe this is undesirable since (in the future when we have RBAC) we don't want to leak any information about what ReleaseComponents and ReleaseArchitectures already exist within Pulp. So Ideally they should always be listed in created_resources irrespective of whether they already existed or were just created. Just like the Release.

To achieve this we probably need to update the task manually when we go through the retrieve case. That being said, I am not sure how easy it is to access the task, given it is created by pulpcore, and not by us.

I think we already have the same wonky behavior when uploading packages as well, so we could leave this for a follow up PR.

pulp / pulp_deb

Create architectures and components when creating a release #1069