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

Quick fix for AnyType ruby bindings #3679

Merged
merged 1 commit into from
Jul 10, 2024
Merged

Quick fix for AnyType ruby bindings #3679

merged 1 commit into from
Jul 10, 2024

Conversation

pedro-psb
Copy link
Member

@pedro-psb pedro-psb commented Jul 9, 2024

Quicker alternative to #3653 for fixing #3639

I must add that this is not the "correct way" of fixing it, but its quick and safe (in the sense that it leaves us in a similar state we had before, where all jsonfields were recognized as Objects). Its not the correct way because:

  • the schema type is misleading, which is not helpful for users. E.g, its says its an Object (openapi type) when its an Array example
  • its not doing basic structural validation (often we know exactly what the json structure is)

Reference

@github-actions github-actions bot added the multi-commit Added when a PR consists of more than one commit label Jul 9, 2024
The JSONField was yielding an empty type on the schema, which
broke bindings generation.

This replaces drf serializers.JSONField with a custom one that is an
OpenApi 'object' type.

Closes: pulp#3639
@github-actions github-actions bot removed the multi-commit Added when a PR consists of more than one commit label Jul 9, 2024
@pedro-psb
Copy link
Member Author

Given the diff in the clients specs, the unexpected presence of the AnyType in the ruby client is the only cause of the problem.

We don't test the bindings on PR, so to assert that manually you can:

  • install the local PR checkout in a venv
  • run pulpcore-manager openapi --bindings --component rpm --file rpm-api.json
  • move it to a recent pulp-openapi-generator repo local checkout
  • run ./gen-client.sh rpm-api.json rpm ruby pulp_rpm
  • grep inside pulp_rpm-ruby-client to assert there is no AnyType anywhere

@dralley
Copy link
Contributor

dralley commented Jul 10, 2024

So this seems reasonable to me, but @mdellweg is probably a better judge of correctness. I'd like us to file an issue to revert this change once a "correct" fix is ready though

Copy link
Member

@mdellweg mdellweg left a comment

Choose a reason for hiding this comment

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

Yes, the would work in isolation for these pulp_rpm fields.

Maybe you want to call it JSONObjectField.

@ggainey ggainey merged commit 2e0c293 into pulp:main Jul 10, 2024
16 checks passed
Copy link

patchback bot commented Jul 10, 2024

Backport to 3.27: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.27/2e0c2933675c0ce41d8ac5c801f8d0dbc54349d8/pr-3679

Backported as #3680

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

Copy link

patchback bot commented Oct 21, 2024

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

❌ Failed to cleanly apply 2e0c293 on top of patchback/backports/3.26/2e0c2933675c0ce41d8ac5c801f8d0dbc54349d8/pr-3679

Backporting merged PR #3679 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/2e0c2933675c0ce41d8ac5c801f8d0dbc54349d8/pr-3679 upstream/3.26
  4. Now, cherry-pick PR Quick fix for AnyType ruby bindings #3679 contents into that branch:
    $ git cherry-pick -x 2e0c2933675c0ce41d8ac5c801f8d0dbc54349d8
    If it'll yell at you with something like fatal: Commit 2e0c2933675c0ce41d8ac5c801f8d0dbc54349d8 is a merge but no -m option was given., add -m 1 as follows instead:
    $ git cherry-pick -m1 -x 2e0c2933675c0ce41d8ac5c801f8d0dbc54349d8
  5. At this point, you'll probably encounter some merge conflicts. You must
    resolve them in to preserve the patch from PR Quick fix for AnyType ruby bindings #3679 as close to the
    original as possible.
  6. Push this branch to your fork on GitHub:
    $ git push origin patchback/backports/3.26/2e0c2933675c0ce41d8ac5c801f8d0dbc54349d8/pr-3679
  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.

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.

4 participants