Feature/tir rm dyn data add helm status#1143
Conversation
If helm is used for deployment, the components deployed by helm are no longer listed as toplevel elements in the TIR. Instead the helm release is represented by its status so that users will see tables under: Deployment Resource: <helm-release-name>-deploymentMean Deployment Resource: <helm-release-name>-deploymentStatus The DeploymentStatus has rows surfacing the releaseName, releaseRevision, namespace, deployStatus, deployDescription, lastDeployed and resources. Here an example from the resources: "Deployment: backend-helm-monorepo-chart-component-a, backend- helm-monorepo-chart-component-b, Service: backend-helm-monorepo- chart" Non-Helm deployments are surfaced as before. Further change notes: - To avoid CPS issues in Jenkins all types were using their own file and the Immutable annotation was avoided. - Type HelmStatusData captures helm status with high fidelity whereas HelmStatusSimpleData captures shallower information in particular omitting the details of resources except their names and kinds. This approach simplifies the json Helm status json parsing code while keeping the consuming code relatively lean. In the deployment artifact the simplified helmStatus is kept under key 'helmStatus' in the deploymentMean information. Which is also written to `ods-deployments.json` by DeploymentDescriptor. - PodData fields capturing dynamic data where removed, except for the podName which has a prefix useful for correlating dynamic data with pods. At the moment this is still needed for both tailor and helm deployments. The code which finds this named pods did require retry logic even for helm installs using --atomic.
+ oc -n kraemerh-dev get pod -l app.kubernetes.io/instance=backend-helm-monorepo -o json expected to call org.ods.util.JsonLogUtil.debug but wound up catching org.ods.util.Logger.getDebugMode; see: https://jenkins.io/redirect/pipeline-cps-method-mismatches/
jafarre-bi
left a comment
jafarre-bi
left a comment
There was a problem hiding this comment.
Several changes and simplifications are needed. Please, also check that all required unit tests are implemented. There is no unit test for HelmStatusSimpleData.fromJsonData.
| } | ||
|
|
||
| @NonCPS | ||
| Map<String, Object> toMap() { |
There was a problem hiding this comment.
Is this really used? And if it is used, do we really need this? Having this class is a proper alternative to using maps.
There was a problem hiding this comment.
Yes, this is needed at least in order to create a Map value for another Map that will be added to a Context.
|
|
||
| @NonCPS | ||
| String toString() { | ||
| return toMap().toMapString() |
There was a problem hiding this comment.
We can directly build the string instead. Probably the toMap method isn't necessary and will be removed.
There was a problem hiding this comment.
This is needed for GStrings, see: #1143 (comment)
| } | ||
|
|
||
| @NonCPS | ||
| private static Tuple2<String, String> extractResource( |
There was a problem hiding this comment.
It's very difficult to understand this method. There must be a much simpler and clearer way to do this, which may even not need a method returning a tuple. This needs rewriting.
There was a problem hiding this comment.
The logic for this method is once again related to throwing exceptions with messages reporting what attributes are missing, see: #1143 (comment)
|
|
||
| @SuppressWarnings(['Instanceof']) | ||
| @NonCPS | ||
| private static Tuple2<List<String>, List<String>> collectMissingStringAttributes( |
There was a problem hiding this comment.
If an attribute is missing, its value will be null. The users of this class will decide what to do with it.
| import groovy.transform.TypeChecked | ||
|
|
||
| @TypeChecked | ||
| class HelmStatusSimpleData { |
There was a problem hiding this comment.
This is just a typical JavaBean holding a set of properties. It's reasonable to have a builder from a Map, but otherwise, no other logic or validation should be done. The users of this data will validate the property values when needed.
There was a problem hiding this comment.
I'd like to ask for some clarification on this, regarding use cases for this HelmStatusSimpleData.
If this class is intended to be used as a "typed" JavaBean object in order to guarantee type-safety and data integrity, excluding the logic for converting to/from Map, what's the purpose for the rest of the logic?
There was a problem hiding this comment.
Hi @anteloro-boeh I am back now and if needed could answer questions about what I had in mind when writing this code in the first place. But perhaps this was already sorted out.?
anteloro-boeh
force-pushed
the
feature/tir-rm-dyn-data-add-helm-status
branch
from
8c9643b to
c762b3b
Compare
# Conflicts: # CHANGELOG.md # src/org/ods/component/HelmDeploymentStrategy.groovy # src/org/ods/orchestration/usecase/LeVADocumentUseCase.groovy # test/groovy/org/ods/orchestration/usecase/LeVADocumentUseCaseSpec.groovy
|
|
||
| def modifier = { byte[] document -> | ||
| codeReviewReport | ||
| ? this.pdf.merge(this.steps.env.WORKSPACE as String, [document] + codeReviewReport) |
There was a problem hiding this comment.
Isn't this the same as [document, codeReviewReport]?
There was a problem hiding this comment.
Not in this case, given that codeReviewReport is of type List<byte[]>
which gives:
[document] + codeReviewReport -> [byte[], byte[], byte[], ...] // we want this
[document, codeReviewReport] -> [byte[], List<byte[]>] // but not this
An alternative would be: [document, *codeReviewReport]
but spread operator '*' is not frequently used in this codebase.
3 participants
This is to supersede PR #1135. It is designed to be merged with the changes in opendevstack/ods-document-generation-templates#146.
Compared to PR#1135 some code was removed to reduce complexity and also addresses review comments.
The initial PR anticipated that we would want to take advantage of Pod related information in the helm status. However this information is not present for DeploymentConfig. Another change was that the helm release status is now surfaced with their own table replacing the deployment resource centric format of the TIR for tailor based deployment. Only a single helm release per component is anticipated and it is displayed like this:
Deployment Status
Table surfacing the releaseName, releaseRevision, namespace, deployStatus, deployDescription, lastDeployed and resources.
Deployment Mean
Table surfacing helm parameters.
Further change notes: