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

fix: add missing nop logic for custom properties plugin #738

Open
wants to merge 1 commit into
base: main-enterprise
Choose a base branch
from

Conversation

PendaGTP
Copy link
Contributor

@PendaGTP PendaGTP commented Jan 10, 2025

Fixes #734

Implements proper nop (dry-run) logic handling for Custom PropertIes plugin, aligning with documentation and other plugins.

Changes

  • Add nop mode support for property modifications
  • Add paginate for gettings all custom properties
  • Switch to built-in Octokit functions instead of github.request
  • Update API endpoint format from /repos/:org/:repo/properties/values to /repos/{owner}/{repo}/properties/values for consistency with GitHub API
  • Refactor property modification logic into a single modifyProperty method
  • Add debug logging for better troubleshooting
  • Normalize custom property names (from GH API) to lowercase to prevent case-sensitivity comparison issues
  • Update test cases

Note

Currently, organization properties are not fetched from the GH Organization API. When modifying repo custom properties, this results in a 422 error if the property isn't defined at the org level. Future improvements could:

  • Handle missing org properties to enable preview changes in nop mode
  • Add support for defining Organization custom properties (requires broader changes)

@PendaGTP PendaGTP force-pushed the fix/add-missing-nop-of-custom-properties branch from 79080d0 to f25def7 Compare January 20, 2025 13:31
@wernerb
Copy link

wernerb commented Jan 21, 2025

It has a bug where in the PR it states:

error TypeError: Cannot read properties of undefined (reading 'toLowerCase') in CustomProperties for repo: {\\\"owner\\\":\\\"x\\\",\\\"repo\\\":\\\"y\\\"} entries [{\\\"name\\\":\\\"type\\\",\\\"value\\\":\\\"myvalue\\\"}]\",

@PendaGTP
Copy link
Contributor Author

PendaGTP commented Jan 22, 2025

It has a bug where in the PR it states:

error TypeError: Cannot read properties of undefined (reading 'toLowerCase') in CustomProperties for repo: {\\\"owner\\\":\\\"x\\\",\\\"repo\\\":\\\"y\\\"} entries [{\\\"name\\\":\\\"type\\\",\\\"value\\\":\\\"myvalue\\\"}]\",

@wernerb I have not been able to reproduce this issue. Cloud you please share your config files?

I've tried the following scenarios:

  • add/remove/update custom_properties in settings.yml
  • add/remove/update custom_properties in repos/repo.yml
  • adding and remove org custom properties through the UI
  • updatating repository custom properties values through the UI

Until a more comprehensive solution is implemented (#719), we could consider adding basic validation:

  • Validate that entries is an array
  • Verify each array item is an object with a defined name property of type string (and value must be null/undefined or type of string)

Thanks!

@wernerb
Copy link

wernerb commented Jan 22, 2025

@PendaGTP I use the official docker image 2.15-rc.1 as there is a problem with the newer images (see latest issue where i get a 404 on using the image). I override custom_properties.js and mount that in the container. In a suborg I modify a custom property and the error happens and is seen in the PR. If you need help reproducing feel free to hit me up on github in DMs. Unfortunately this is high priority for us.

It is such high-priority, that I am tempted to do away with our hosted flow and use the action flow instead as it seems it is more supported?

The custom property is a "text" type property nothing really special with it

@PendaGTP
Copy link
Contributor Author

@wernerb see #743 (comment)

@PendaGTP
Copy link
Contributor Author

Additionally, there are currently reported issues with the full-sync script used in GitHub Actions (GHA), so I recommend against migrating to it at this time.
Related issues: #739 #378 #709

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.

Custom properties are applied during pull request
2 participants