-
Notifications
You must be signed in to change notification settings - Fork 311
upgrade command add three-way-merge option #304
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
Conversation
…ate vs desired state
|
@databus23 @mumoshu can I humbly ask you folks to take a look? 🙏 This might be a step in the right direction to solve #176 |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
| return fmt.Errorf("Failed to render chart: %s", err) | ||
| } | ||
|
|
||
| if d.threeWayMerge { |
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.
Probably L171-L193 doesn't need to be run when we enter the 3-way merge mode?
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.
Nevermind. It turned out correct as we still render the chart at L190 to be used when building K8s objects at L207.
mumoshu
left a comment
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 have one concern but this looks great for a starter. Thanks a lot for your patience and contribution!
This adds a dedicated envvar to enable the three-way-merge diff added in #304. It should be useful when you are using helm-diff from another command or script and you dont have ability to specify the --three-way-merge flag without modifying the command/script but you cant due to various reasons.
This adds a dedicated envvar to enable the three-way-merge diff added in #304. It should be useful when you are using helm-diff from another command or script and you dont have ability to specify the --three-way-merge flag without modifying the command/script but you cant due to various reasons.
|
Yeah maybe in coming weeks. |
|
FYI, I've added a feature to enable this via an envvar. See #336 for more information. |
upgrade command add three-way-merge option to show diff for actual state vs desired state #176
prepared resource for testing
1、last release:
2、target release:
normal diff vs diff with
--three-way-mergeoption1、manual change:
preStoptime from 5 to 10readinessProbeService2、normal diff output:
default, base-app-go, Deployment (apps) has changed: ... - '-c' - 'sleep 5' livenessProbe: - httpGet: - path: /healthz - port: 1323 initialDelaySeconds: 10 periodSeconds: 60 + tcpSocket: + port: 1323 readinessProbe: httpGet: path: /healthz ... - name: CLUSTER value: dev - name: COUNT - value: "3" + value: "4" volumes: - hostPath: path: /etc/localtime3、diff output with
--three-way-mergeoption:default, base-app-go, Deployment (apps) has changed: ... meta.helm.sh/release-name: base-app-01 meta.helm.sh/release-namespace: default creationTimestamp: "2021-09-01T06:29:39Z" - generation: 8 + generation: 9 labels: app: base-app-go app.kubernetes.io/managed-by: Helm ... - env: - name: CLUSTER value: dev - name: COUNT - value: "3" + value: "4" image: "xxxxx" imagePullPolicy: IfNotPresent lifecycle: ... command: - /bin/sh - -c - - sleep 10 + - sleep 5 livenessProbe: failureThreshold: 3 - httpGet: - path: /healthz - port: 1323 - scheme: HTTP initialDelaySeconds: 10 periodSeconds: 60 successThreshold: 1 + tcpSocket: + port: 1323 timeoutSeconds: 1 name: base-app-go ports: - containerPort: 1323 name: http-1323 protocol: TCP + readinessProbe: + failureThreshold: 3 + httpGet: + path: /healthz + port: 1323 + scheme: HTTP + initialDelaySeconds: 10 + periodSeconds: 3 + successThreshold: 1 + timeoutSeconds: 1 resources: {} terminationMessagePath: /dev/termination-log terminationMessagePolicy: File ... default, base-app-go, Service (v1) has been added: - + apiVersion: v1 + kind: Service + metadata: + labels: + app: base-app-go + app.kubernetes.io/managed-by: Helm + helm.sh/chart: base-app-1.0.0 + owner: somebody + team: ops + name: base-app-go + namespace: default + spec: + ports: + - name: http-1323 + port: 1323 + protocol: TCP + targetPort: 1323 + selector: + app: base-app-go + type: ClusterIP