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

Update application subcharts when subcharts are implicit #258

Merged
merged 7 commits into from
May 28, 2021

Conversation

nitishm
Copy link
Contributor

@nitishm nitishm commented May 19, 2021

Fixes #257

Update application subcharts when subcharts are implicit

Signed-off-by: Nitish Malhotra nitish.malhotra@gmail.com

…implicitly defined

Signed-off-by: Nitish Malhotra <nitish.malhotra@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented May 19, 2021

Codecov Report

Merging #258 (9122241) into main (0d5db50) will increase coverage by 0.03%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #258      +/-   ##
==========================================
+ Coverage   31.88%   31.92%   +0.03%     
==========================================
  Files           6        6              
  Lines        1016     1018       +2     
==========================================
+ Hits          324      325       +1     
- Misses        664      665       +1     
  Partials       28       28              
Impacted Files Coverage Δ
controllers/appgroup_reconciler.go 50.31% <50.00%> (-0.01%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d5db50...9122241. Read the comment docs.

Copy link
Contributor

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

Not sure what happens here

@@ -99,7 +99,22 @@ func (r *ApplicationGroupReconciler) reconcileApplications(l logr.Logger, appGro
}

if appCh.Dependencies() != nil {
isImplicitSubchartsList := false
// Update the application spec with a list of subcharts with no inter-dependence
if len(application.Spec.Subcharts) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if we specify some subcharts (but not all) and we want the ones not specified to not wait?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, can you add a testcase for this scenario?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point - I will add handlers for that case. Sure - will add testcases !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jonathan-innis I made the change and verified it manually. We still need to add tests for this scenario but I will track that in a separate issue since we need this to cut our next release. Let me know if that works and then I can create the other test issue

Signed-off-by: Nitish Malhotra <nitish.malhotra@gmail.com>
Signed-off-by: Nitish Malhotra <nitish.malhotra@gmail.com>
Signed-off-by: Nitish Malhotra <nitish.malhotra@gmail.com>
@nitishm nitishm merged commit 9a1c2f7 into Azure:main May 28, 2021
@nitishm nitishm deleted the nitishm/bug/257/implicit-subcharts-staged branch May 28, 2021 17:06
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.

Subcharts not getting deployed if not explicitly mentioned in the ApplicationGroup object
3 participants