Skip to content

Conversation

@ShashwatDadhich
Copy link
Contributor

@ShashwatDadhich ShashwatDadhich commented Aug 29, 2023

Description

This feature will enable users to compare the generated manifest if they are doing some changes or updating the chart version so they can see if anything getting changed would not affect their existing micro-services. Users will be quite confident while updating the chart version and they will keep their microservice to the latest version.

While making changes in the Deployment template users do not feel confident about the impact of the changes

DevOps team may not want developers to make changes in production directly as they may make inadvertent errors.

Ability to view and compare the manifest output of values.yaml in Devtron Apps.

🤖 Prototype link
🎨 Design link

Fixes #3503

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Test locally
  • Test by deploying in VM

Checklist:

  • The title of the PR states what changed and the related issues number (used for the release note).
  • Does this PR requires documentation updates?
  • I've updated documentation as required by this PR.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have tested it for all user roles.
  • I have added all the required unit/api test cases.

@ShashwatDadhich ShashwatDadhich changed the title Manifest list test feat: manifest comparision Aug 29, 2023
}
return appList
}

Copy link
Contributor

Choose a reason for hiding this comment

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

revert this line

func (handler AppListingRestHandlerImpl) GetDeploymentsWithCharts(w http.ResponseWriter, r *http.Request) {
vars := mux.Vars(r)
token := r.Header.Get("token")
appId, err := strconv.Atoi(vars["app-id"])
Copy link
Contributor

@gireesh-naidu gireesh-naidu Aug 30, 2023

Choose a reason for hiding this comment

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

make these vars to camelcase

return
}

envId, err := strconv.Atoi(vars["env-id"])
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

return
}
// RBAC enforcer applying
object := handler.enforcerUtil.GetAppRBACName(app.AppName)
Copy link
Contributor

Choose a reason for hiding this comment

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

use func GetAppRBACNameByAppId instead of GetAppRBACName

return
}
// RBAC enforcer applying
object := handler.enforcerUtil.GetAppRBACName(app.AppName)
Copy link
Contributor

Choose a reason for hiding this comment

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

use func GetAppRBACNameByAppId instead of GetAppRBACName

query := "select merged_values_yaml from pipeline_config_override where id = ? ; "
_, err := impl.dbConnection.Query(&result, query, id)
if err != nil {
impl.Logger.Error("error", "error", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

log error message correctly


defaultVersions, err := impl.chartService.ChartRefAutocompleteForAppOrEnv(appId, 0)
if err != nil {
impl.Logger.Errorw("err", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

log error properly

gireesh-naidu
gireesh-naidu previously approved these changes Oct 10, 2023
Copy link
Contributor

@gireesh-naidu gireesh-naidu left a comment

Choose a reason for hiding this comment

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

LGTM

return
}

app, err := handler.pipeline.GetApp(appId)
Copy link
Contributor

Choose a reason for hiding this comment

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

why do u need app here?

common.WriteJsonResp(w, nil, resp, http.StatusOK)
}

func (handler AppListingRestHandlerImpl) GetYaluesAndManifest(w http.ResponseWriter, r *http.Request) {
Copy link
Contributor

Choose a reason for hiding this comment

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

change the func name to GetValuesAndManifest

}

token := r.Header.Get("token")
app, err := handler.pipeline.GetApp(request.AppId)
Copy link
Contributor

Choose a reason for hiding this comment

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

why do u need app here ?

}

type ValuesAndManifestResponse struct {
Data string `json:"data"`
Copy link
Contributor

Choose a reason for hiding this comment

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

please add a comment what kind of data u store here

return
}
// RBAC enforcer applying
object := handler.enforcerUtil.GetAppRBACName(app.AppName)
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this rbac on app level while u fetch the manifest for env level aswell. the RBAC should be conditional.

gireesh-naidu
gireesh-naidu previously approved these changes Oct 11, 2023
vikramdevtron
vikramdevtron previously approved these changes Oct 11, 2023
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 31 Code Smells

No Coverage information No Coverage information
14.2% 14.2% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@ShashwatDadhich ShashwatDadhich merged commit fd8d491 into main Oct 12, 2023
@ShashwatDadhich ShashwatDadhich deleted the manifest-list-test branch October 12, 2023 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature: Comparison of manifest generated by deployment chart version being used

6 participants