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

Allow core/3.55+. #3621

Merged
merged 1 commit into from
Jun 20, 2024
Merged

Allow core/3.55+. #3621

merged 1 commit into from
Jun 20, 2024

Conversation

ggainey
Copy link
Contributor

@ggainey ggainey commented Jun 18, 2024

fixes #3630.

@ggainey
Copy link
Contributor Author

ggainey commented Jun 18, 2024

Will undraft after core/3.55 releases and re-run tests.

@ggainey ggainey marked this pull request as ready for review June 18, 2024 18:57
@ggainey
Copy link
Contributor Author

ggainey commented Jun 18, 2024

Oh look, we have work to do...

@ggainey ggainey force-pushed the 3630_allow_core_355 branch 7 times, most recently from 6e9c111 to 7e1e230 Compare June 18, 2024 23:51
@@ -0,0 +1 @@
Allow installation with pulpcore/3.55.
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 somehow sync up this formulation across plugins?

Also: pulp_rpm seems to choose a new ad-hoc formulation each time, from the changelog:

  • Features: Added pulpcore 3.40 compatibility.
  • Features: Declares (and requires at least) pulpcore/3.25 compatibility.

Why is this suddenly a bugfix and can we become more consistent?

Copy link
Contributor

Choose a reason for hiding this comment

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

I added a comment about this on the pulpcore 3.55 discourse channel as well: https://discourse.pulpproject.org/t/pulpcore-3-55-is-coming-soon/1259/3?u=quba42

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is this suddenly a bugfix and can we become more consistent?

Because my initial thought was that this would be backportable. Alas, no. Good catch, will fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussion from rpm-team-mtg: previous such releases have been features because they relied-on functionality in the breaking-release, and so needed their lowerboud raised. We generally try to allow the widest possible installation/use of pulp_rpm. If you look at the tests here, the current PR works cleanly back to the current LB ofrpm/3.44.1 (see https://github.com/pulp/pulp_rpm/actions/runs/9573762412/job/26468187737#step:21:53). Given that this change can be backported to previous releases, we mark it as bugfix.

That being said - the desire for there to be some similarity/compatibility for changelog entries across plugins is a good one. Will change.

Copy link
Contributor Author

@ggainey ggainey Jun 20, 2024

Choose a reason for hiding this comment

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

Of course, what we're really declaring is "compatibility with pulpcore<3.70". Which isn't what any of your example-changelogs say.

Copy link
Contributor

Choose a reason for hiding this comment

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

Communication wise, I think a changelog entry of "Declared compatibility against pulpcore<3.70" at a point in time when pulpcore 3.55 is latest, has a lot of potential to confuse anyone not intimately acquainted with the Plugin API deprecation policy (most users). So I think something like "Declared compatibility against pulpcore 3.55+" has the following virtues:

  • While less technically accurate than pulpcore<3.70 it is still true.
  • It identifies the actual existing breaking change release that now works.
  • Via the "+" It still hints that there is a range beyond the "3.55" that we also declared compatibility against.

So I quite like this variant. That being said, I think future consistency is more important than the exact schema chosen.

@ggainey ggainey marked this pull request as draft June 20, 2024 15:33
@ggainey ggainey marked this pull request as ready for review June 20, 2024 16:31
@ggainey ggainey enabled auto-merge (rebase) June 20, 2024 19:29
):
"""Test whether one can upload an RPM with non-ascii metadata."""
temp_file = tmp_path / str(uuid.uuid4())
temp_file.write_bytes(requests.get(RPM_WITH_NON_ASCII_URL).content)
artifact = artifacts_api_client.create(temp_file)
artifact = pulpcore_bindings.ArtifactsApi.create(temp_file)
Copy link
Contributor

Choose a reason for hiding this comment

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

I appreciate this cleanup, thanks.

def _create_export(
import_repos=None, export_repos=None, url=RPM_UNSIGNED_FIXTURE_URL, exporter=None
):
if not exporter:
exporter = create_exporter(import_repos, export_repos, url=url)

export_response = exporters_pulp_exports_api_client.create(exporter.pulp_href, {})
export_response = pulpcore_bindings.ExportersPulpExportsApi.create(exporter.pulp_href, {})
Copy link
Contributor

Choose a reason for hiding this comment

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

much much cleaner

@ggainey ggainey merged commit 1c36b39 into pulp:main Jun 20, 2024
15 checks passed
Copy link

patchback bot commented Jun 20, 2024

Backport to 3.27: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.27/1c36b39e2be24f87a97821370ee850cafefddbe6/pr-3621

Backported as #3623

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

Copy link

patchback bot commented Jun 20, 2024

Backport to 3.26: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply 1c36b39 on top of patchback/backports/3.26/1c36b39e2be24f87a97821370ee850cafefddbe6/pr-3621

Backporting merged PR #3621 into main

  1. Ensure you have a local repo clone of your fork. Unless you cloned it
    from the upstream, this would be your origin remote.
  2. Make sure you have an upstream repo added as a remote too. In these
    instructions you'll refer to it by the name upstream. If you don't
    have it, here's how you can add it:
    $ git remote add upstream https://github.com/pulp/pulp_rpm.git
  3. Ensure you have the latest copy of upstream and prepare a branch
    that will hold the backported code:
    $ git fetch upstream
    $ git checkout -b patchback/backports/3.26/1c36b39e2be24f87a97821370ee850cafefddbe6/pr-3621 upstream/3.26
  4. Now, cherry-pick PR Allow core/3.55+. #3621 contents into that branch:
    $ git cherry-pick -x 1c36b39e2be24f87a97821370ee850cafefddbe6
    If it'll yell at you with something like fatal: Commit 1c36b39e2be24f87a97821370ee850cafefddbe6 is a merge but no -m option was given., add -m 1 as follows instead:
    $ git cherry-pick -m1 -x 1c36b39e2be24f87a97821370ee850cafefddbe6
  5. At this point, you'll probably encounter some merge conflicts. You must
    resolve them in to preserve the patch from PR Allow core/3.55+. #3621 as close to the
    original as possible.
  6. Push this branch to your fork on GitHub:
    $ git push origin patchback/backports/3.26/1c36b39e2be24f87a97821370ee850cafefddbe6/pr-3621
  7. Create a PR, ensure that the CI is green. If it's not — update it so that
    the tests and any other checks pass. This is it!
    Now relax and wait for the maintainers to process your pull request
    when they have some cycles to do reviews. Don't worry — they'll tell you if
    any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

@ggainey ggainey deleted the 3630_allow_core_355 branch June 21, 2024 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants