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

feat: Replace Controller role with Billing Admin role #528

Merged
merged 3 commits into from
Feb 15, 2024

Conversation

Ayushi3333
Copy link
Contributor

@Ayushi3333 Ayushi3333 commented Jan 29, 2024

We recently renamed all Controllers to Billing Admins in PR. This needs to be reflected in the api docs.

@Ayushi3333 Ayushi3333 self-assigned this Jan 29, 2024
Copy link

netlify bot commented Jan 29, 2024

Deploy Preview for open-api ready!

Name Link
🔨 Latest commit bdb29d0
🔍 Latest deploy log https://app.netlify.com/sites/open-api/deploys/65ce067217074e000888248d
😎 Deploy Preview https://deploy-preview-528--open-api.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@Ayushi3333 Ayushi3333 changed the title Replace Controller with Billing Admin feat: Replace Controller role with Billing Admin role Feb 13, 2024
@Ayushi3333 Ayushi3333 marked this pull request as ready for review February 13, 2024 10:46
@Ayushi3333 Ayushi3333 requested review from a team as code owners February 13, 2024 10:46
Copy link
Member

@mraerino mraerino left a comment

Choose a reason for hiding this comment

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

this seems like a breaking change? should we have a backwards compatible API that allows Controller as an alias for Billing Admin?

also i think you need to run go generate ./... and commit the generated files

@Ayushi3333
Copy link
Contributor Author

Ayushi3333 commented Feb 13, 2024

this seems like a breaking change? should we have a backwards compatible API that allows Controller as an alias for Billing Admin?

also i think you need to run go generate ./... and commit the generated files

Can you elaborate how this is a breaking change? We no longer accept Controller as a role in the API requests. This change is ensuring Controller is no longer used because it won't work. If you are worried about our UI, netlify-react-ui is sending both roles in the params to support the transition.

Thanks for the second suggestion, I will try to generate the files.

@mraerino
Copy link
Member

mraerino commented Feb 13, 2024

Can you elaborate how this is a breaking change? We no longer accept Controller as a role in the API requests. This change is ensuring Controller is no longer used because it won't work. If you are worried about our UI, netlify-react-ui is sending both roles in the params to support the transition.

yeah, exactly what i meant. we made a breaking change to our API since existing code from customers that might have used Controller now fails.
i'm bringing this up here because everything that we document in open-api is supposed to be fairly stable, since once it's documented publicly, customers will start relying on it. if we hadn't documented it, the breaking change wouldn't be too significant since we don't need to provide stability when we expect a method to only be used in our UI.

we have this distinction deliberately. there is an internal open api spec that contains all methods that our API offers, not just the ones we want to provide to customers

@Ayushi3333
Copy link
Contributor Author

Ayushi3333 commented Feb 14, 2024

Can you elaborate how this is a breaking change? We no longer accept Controller as a role in the API requests. This change is ensuring Controller is no longer used because it won't work. If you are worried about our UI, netlify-react-ui is sending both roles in the params to support the transition.

yeah, exactly what i meant. we made a breaking change to our API since existing code from customers that might have used Controller now fails. i'm bringing this up here because everything that we document in open-api is supposed to be fairly stable, since once it's documented publicly, customers will start relying on it. if we hadn't documented it, the breaking change wouldn't be too significant since we don't need to provide stability when we expect a method to only be used in our UI.

we have this distinction deliberately. there is an internal open api spec that contains all methods that our API offers, not just the ones we want to provide to customers

You are absolutely right that this would be a problem if this was being actively used. There are 85 non-netlify Billing Admins all time and I have a strong suspicion they were not created through the API but rather through the UI.

I do not think this role is being created by the users programatically. To substantiate my claim, I tried looking in Humio but I cannot find a way to differentiate "user-generated" requests vs our UI. I can find some data to confirm once I figure out how to obtain it.

EDIT - I found a way to query requests not coming from our FE app and as you can see here, in the last 3 months (our Humio retention period), there's not been a single request to create or update Controller roles. I think it is safe to say users are not using these endpoints programatically and it is safe to update it in the documentation.

@Ayushi3333 Ayushi3333 force-pushed the ayushi/ab-522-replace-controller-with-ba branch from d31a509 to c1a7687 Compare February 15, 2024 11:45
@Ayushi3333 Ayushi3333 enabled auto-merge (squash) February 15, 2024 11:58
@Ayushi3333 Ayushi3333 merged commit c24b3a3 into master Feb 15, 2024
15 checks passed
@Ayushi3333 Ayushi3333 deleted the ayushi/ab-522-replace-controller-with-ba branch February 15, 2024 12:41
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.

None yet

3 participants