-
Notifications
You must be signed in to change notification settings - Fork 15
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
BCDA-8631: Create group lambda #1039
base: main
Are you sure you want to change the base?
Conversation
And a reminder that the workflows wont work until you have created/merged the supporting infrastructure in ab2d-bcda-dpc-platform. |
yup! have a PR open on that one as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few nitpicks but more importantly I think we should definitely beef up the test suite if we arent reusing existing code.
bcda/lambda/create_group/main.go
Outdated
if err != nil { | ||
return err | ||
} | ||
} else if match, err := regexp.MatchString("[0-9a-f]{6}-([0-9a-f]{4}-){3}[0-9a-f]{12}", data.ACO_ID); err == nil && match { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Do we want/need to support uuids? It seems like by far the less useful form?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also caught my eye too. We end up just using the CMS ID at the end of the operation and not using the UUID, except to validate the ACO. I was going to throw out the UUID but I think it's possible the SGAs use them when creating groups. I'd be happy to remove but I think we would just need to give them a heads up that group creation may change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGAs wont be using our lambdas though right?
Im also noticing that you are calling using a SSASClient to call CreateGroup(), I had to a lot of set up to get that working on my create aco creds branch: https://github.com/CMSgov/bcda-app/pull/1038/files (setting up a variety of env vars). This client eventually sends requests to SSAS api, which is currently timing out, related ticket: https://jira.cms.gov/browse/PLT-903
🎫 Ticket
https://jira.cms.gov/browse/BCDA-8631
🛠 Changes
Lambda has been added so that we can call it to create groups, ad hoc.
ℹ️ Context
Done as part of the effort to migrate away from jenkins.
🧪 Validation
Tests & workflows passing; additional testing blocked until lambda can be deployed.