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

Re-generate generated API to add new r/w/d organization role. #644

Merged
merged 2 commits into from
Oct 30, 2024

Conversation

NullHypothesis
Copy link
Contributor

This PR re-generates the generated API to add the new read/write/delete organization role that was introduced in TileDB-Inc/TileDB-Cloud-API-Spec#483. In addition, this PR makes a minor change to make update_generated.sh support macOS.


Note that I re-generated the full API. As far as I can tell, the only change we need is this one.

@NullHypothesis NullHypothesis changed the title Re-generate generated API to add new r/w/d organization permissions. Re-generate generated API to add new r/w/d organization role. Sep 13, 2024
@sgillies
Copy link
Collaborator

@NullHypothesis can you re-regenerate and push to see if the conflicts are resolved?

@NullHypothesis
Copy link
Contributor Author

@NullHypothesis can you re-regenerate and push to see if the conflicts are resolved?

Thanks for taking a look! I'll pick this back up once the corresponding REST PR is merged.

@NullHypothesis NullHypothesis force-pushed the phw/sc-51873/add-new-read-write-delete-permission branch 2 times, most recently from adcf1f8 to 246c83b Compare September 27, 2024 13:03
@NullHypothesis NullHypothesis marked this pull request as ready for review September 30, 2024 13:15
@NullHypothesis NullHypothesis marked this pull request as draft September 30, 2024 14:28
This adds the newly-introduced read/write/delete organization role,
along with an unrelated change to the spec.

I re-generated the API by running:

    ./update_generated.sh /path/to/TileDB-Cloud-API-Spec
@NullHypothesis NullHypothesis force-pushed the phw/sc-51873/add-new-read-write-delete-permission branch from 246c83b to b35d7b2 Compare September 30, 2024 15:07
@NullHypothesis
Copy link
Contributor Author

@sgillies This is ready for review but we should hold off on merging until the corresponding REST PR is deployed. I'll un-draft this PR once that's the case.

Regarding the code itself: I noticed that re-generating the API on macOS results in a large number of changes while running it on Linux results in exactly what we'd expect. I therefore taught the script to abort if it's run on a non-Linux platform.

Copy link
Collaborator

@sgillies sgillies left a comment

Choose a reason for hiding this comment

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

@NullHypothesis 👍

Do you understand why we see extra changes on macOS even with the same generator version?

This makes it possible to run update_generated.sh on macOS without
modifications.
@NullHypothesis NullHypothesis force-pushed the phw/sc-51873/add-new-read-write-delete-permission branch from b35d7b2 to 8631bac Compare September 30, 2024 20:48
@NullHypothesis
Copy link
Contributor Author

Do you understand why we see extra changes on macOS even with the same generator version?

I took another look and realized that this was (mostly) my own fault. I think I didn't properly run the pre-commit hook. That said, one still needs to use GNU sed instead of the default sed on macOS for this to work, which should be fixed in 8631bac. After that, the output is nearly identical to what I'm getting on Linux, except these VERSION changes we don't really matter:

Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	deleted:    src/tiledb/cloud/_common/api_v2/.openapi-generator/VERSION
	deleted:    src/tiledb/cloud/rest_api/.openapi-generator/VERSION

Untracked files:
  (use "git add <file>..." to include in what will be committed)
	src/tiledb/cloud/_common/api_v2/VERSION
	src/tiledb/cloud/rest_api/VERSION

@NullHypothesis NullHypothesis marked this pull request as ready for review October 22, 2024 18:01
@NullHypothesis
Copy link
Contributor Author

@sgillies We should be ready to merge this PR. We deployed the new read/write/delete role in REST earlier today.

@sgillies sgillies merged commit 74f0601 into main Oct 30, 2024
18 checks passed
@sgillies sgillies deleted the phw/sc-51873/add-new-read-write-delete-permission branch October 30, 2024 19:16
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