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

Add CreateOrUpdateCustomPropertyValues for repositories #3102

Closed
wants to merge 3 commits into from

Conversation

HariCharanK
Copy link
Contributor

Fixes: #3101

Copy link

google-cla bot commented Mar 15, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link

codecov bot commented Mar 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.87%. Comparing base (2b8c7fa) to head (86b134d).
Report is 23 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3102      +/-   ##
==========================================
- Coverage   97.72%   92.87%   -4.85%     
==========================================
  Files         153      170      +17     
  Lines       13390    11416    -1974     
==========================================
- Hits        13085    10603    -2482     
- Misses        215      723     +508     
  Partials       90       90              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

According to https://docs.github.com/en/rest/repos/custom-properties?apiVersion=2022-11-28#create-or-update-custom-property-values-for-a-repository,
the value field can be a string or an array (of strings maybe?), or alternatively it can be null to actually delete that property.

In this repo, we typically try to mirror the functionality of the official documentation in order to hopefully minimize the confusion. So a reader of the official docs should hopefully be able to understand how to use the endpoints in this repo.

By not allowing null (by making value a *string), I'm thinking this change might be confusing to a user... because they will be unable to delete a property that they created with this API.

Additionally, we need to think about how we could support either a string value or an array of strings value.

I don't have the answers at the moment. Maybe we need to think about this one and/or see if there are any similar examples in this repo (I can't think of any off-hand).

@HariCharanK
Copy link
Contributor Author

Oh, right, I overlooked the array part. Thanks for pointing it out!

I'll think of an approach.

@HariCharanK
Copy link
Contributor Author

Hi @gmlewis ,

meta:operation PATCH /repos/{owner}/{repo}/properties/values is missing in openapi_operations.yaml

I did an update to it using, script/metadata.sh update-openapi

It bumped ghes from 3.11 to 3.12 in the entire file and added the required PATCH end-point. But it also removed a couple of end-points.

They are
GET /repositories/{repository_id}/environments/{environment_name}/variables
GET /repositories/{repository_id}/environments/{environment_name}/variables/{name}
and variations of these with POST, PATCH and DELETE

These are being used in github/action_variables.go (line 87, for example, and other places in the same file)

I've checked up the github documentation and looks like the endpoint has changed to
/repos/{owner}/{repo}/environments/{environment_name}/variables

{owner} was added, but I haven't tested if the older one was deprecated or doesn't work anymore.

Should I update this in action_variables.go ?
I can create a separate issue for the same.

I cant proceed to work on the current issue, without this being resolved.

Another way is I can manually add the PATCH path to openapi_operations.yaml under the operation_overrides section. Not sure if this is a good thing to do.

I'd like to hear your thoughts.

@gmlewis
Copy link
Collaborator

gmlewis commented Mar 15, 2024

I'd like to hear your thoughts.

Let me see if I can update it in a separate PR that you will hopefully be able to merge in and be unblocked.

@HariCharanK
Copy link
Contributor Author

Sure, works.

Can I do that tho? 😅
It'll be a learning experience.

@gmlewis
Copy link
Collaborator

gmlewis commented Mar 15, 2024

Can I do that tho? 😅
It'll be a learning experience.

You are welcome to do that, but it sounds like you already tried that and had problems, so I was going to investigate.

Our OpenAPI tools are relatively new (considering the age of this repo), and written by another contributor, and I'm not that familiar with running them myself, so I thought I would dig into it a bit and also look at the official GitHub v3 API docs to see if the removed endpoints have actually been removed or if there is some other problem.

So sure, we can investigate this in parallel if you would like, but I still better look into it from my side to get a better understanding of what exactly is going wrong. 😄

@gmlewis
Copy link
Collaborator

gmlewis commented Mar 15, 2024

OK, I'm able to duplicate your findings above... now to research the official GitHub v3 API docs to figure out what happened with these affected endpoints...

@gmlewis
Copy link
Collaborator

gmlewis commented Mar 15, 2024

Digging deeper, it looks like the endpoints for these with problems were actually moved by the official GitHub v3 API documentation and now need updating.

@HariCharan-001 - if you would like to modify these 5 endpoints to their new locations in a separate PR, that would be great!

$ ./script/metadata.sh update-go
could not find operation "GET /repositories/{repository_id}/environments/{environment_name}/variables" in openapi_operations.yaml
could not find operation "GET /repositories/{repository_id}/environments/{environment_name}/variables/{name}" in openapi_operations.yaml
could not find operation "POST /repositories/{repository_id}/environments/{environment_name}/variables" in openapi_operations.yaml
could not find operation "PATCH /repositories/{repository_id}/environments/{environment_name}/variables/{name}" in openapi_operations.yaml
could not find operation "DELETE /repositories/{repository_id}/environments/{environment_name}/variables/{name}" in openapi_operations.yaml

If not, I can do it. Your call.

@HariCharanK
Copy link
Contributor Author

Thanks a lot for confirming @gmlewis 😄

I'll work on it.

@HariCharanK
Copy link
Contributor Author

Will create a new PR on top of new changes I made in #3104
Closing this.

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.

Changing custom properties
2 participants