-
Notifications
You must be signed in to change notification settings - Fork 76
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
Default dag deploy for hosted #1345
Conversation
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.
the actual logical change seems good to me. Left a minor nit on variable declaration, and also you would need to fix the failing lint & test checks
@@ -33,6 +33,7 @@ var ( | |||
errInvalidValue = errors.New("is not valid") | |||
errNotPermitted = errors.New("is not permitted") | |||
canCiCdDeploy = deployment.CanCiCdDeploy | |||
dagDeploy bool |
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: this should be part of CreateOrUpdate
function logic, since this variable is local to that function only
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.
it's used in CreateOrUpdate and getCreateOrUpdateInput
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.
ahh, it's better to pass the variable as a function argument to getCreateOrUpdateInput
instead of using a global variable. That way function is more readable
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.
okay fixed
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1345 +/- ##
==========================================
- Coverage 86.92% 86.90% -0.02%
==========================================
Files 114 114
Lines 13872 13889 +17
==========================================
+ Hits 12058 12070 +12
- Misses 1083 1085 +2
- Partials 731 734 +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.
Can we bump that coverage?
* dag deploy enabled by default in hosted * dag deploy enabled by default in hosted from file * fix from file * fix lint * fix test * fix test * fix test * fix test * update test coverage
Description
dag deploy enabled by default for hosted
🎟 Issue(s)
🧪 Functional Testing
manual testing in dev in a both a hybrid and hosted org
📸 Screenshots
📋 Checklist
make test
before taking out of draftmake lint
before taking out of draft