-
Notifications
You must be signed in to change notification settings - Fork 79
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 concurrency check for hosted deployments #1279
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1279 +/- ##
==========================================
- Coverage 87.53% 87.52% -0.02%
==========================================
Files 110 110
Lines 13020 13057 +37
==========================================
+ Hits 11397 11428 +31
- Misses 949 952 +3
- Partials 674 677 +3
☔ View full report in Codecov by Sentry. |
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.
functionally looks ok to me. Once any of us has some free cycles (hopefully 🙏 ), we could look to tackle the lining exceptions added in our codebase
@@ -184,10 +184,12 @@ func CreateOrUpdate(inputFile, action string, client astro.Client, coreClient as | |||
// It returns an error if getting default options fail. | |||
// It returns an error if worker-queue options are not valid. | |||
// It returns an error if node pool id could not be found for the worker type. | |||
func getCreateOrUpdateInput(deploymentFromFile *inspect.FormattedDeployment, clusterID, workspaceID, action string, existingDeployment *astro.Deployment, nodePools []astrocore.NodePool, client astro.Client) (astro.CreateDeploymentInput, astro.UpdateDeploymentInput, error) { | |||
func getCreateOrUpdateInput(deploymentFromFile *inspect.FormattedDeployment, clusterID, workspaceID, action string, existingDeployment *astro.Deployment, nodePools []astrocore.NodePool, client astro.Client) (astro.CreateDeploymentInput, astro.UpdateDeploymentInput, error) { //nolint |
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: I think at some point we might have to move to using a struct instead of accepting such a large number of inputs. Also might have look at breaking down such large functions, since we have already put linting exception at few places
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.
I agree. This has been on my mind too.
Description
Fix concurrency check for hosted deployments
🎟 Issue(s)
Related #1267
🧪 Functional Testing
📸 Screenshots
📋 Checklist
make test
before taking out of draftmake lint
before taking out of draft