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(organization): add async option to organizations endpoint to support onboarding of large organizations #591

Closed
wants to merge 3 commits into from

Conversation

maratsal
Copy link
Collaborator

@maratsal maratsal commented Jan 10, 2025

This PR is to address organizations onboarding with thousands of accounts, where previous API endpoint would timeout. ?async=true option was introduced to API, but terraform provider update was missing.

This code has been tested with 900 accounts organizaton onboarding and all worked fine.

@nkraemer-sysdig
Copy link
Contributor

Its worth noting that this change will affect all cloud providers (not just AWS). I'm checking with the team to make sure there are no concerns there

@nkraemer-sysdig nkraemer-sysdig requested a review from a team January 13, 2025 18:23
@maratsal maratsal force-pushed the feat-support-async-call branch from 52e87ea to 4d0cab1 Compare January 13, 2025 18:26
@maratsal
Copy link
Collaborator Author

I tested those manually and it was working fine. Also added async option to another endpoint as well (needed for PUT calls), can be done in more elegant way by concatenating URL in place only for 2 function: CreateOrganizationSecure and UpdateOrganizationSecure. But this way works too.

And finally I was thinking that we likely can implement this by using async option on the api endpoint by default.

Copy link
Contributor

@cgeers cgeers left a comment

Choose a reason for hiding this comment

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

This runs counter to the current plans in this area, I do not support merging this.

@maratsal
Copy link
Collaborator Author

@cgeers which part you don't like? Should i redo it and introduce new option to set async parameter?

@cgeers
Copy link
Contributor

cgeers commented Jan 16, 2025

@maratsal Here are the problems with this change:

  • the API behavior you're relying on is planned to be removed in short order
  • the change here is broad and sweeping and affects things far outside your intended scope
  • if merged as is, QA tests will begin failing and it'll be up to someone to fix all of that

I think the best option here is to close this PR and let the team deliver the feature that supersedes all these workarounds. If that's not acceptable for you, instead of changing the default for or all organizations, make a more targeted change. If you were to control this query param with an undocumented environment variable, for example, while leaving the default behavior as is, that would be acceptable to merge, IMO.

@maratsal
Copy link
Collaborator Author

@cgeers sounds good, I pushed changes to the code to set async=true parameter based on env variable only. Please let me know if this approach will work.

@maratsal
Copy link
Collaborator Author

closing this PR in favour of #594 to be able to finish tests.

@maratsal maratsal closed this Jan 17, 2025
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.

3 participants