Skip to content
This repository was archived by the owner on Oct 11, 2024. It is now read-only.

Remove kompose k8 cruft #1425

Merged
merged 2 commits into from
Jan 6, 2020
Merged

Remove kompose k8 cruft #1425

merged 2 commits into from
Jan 6, 2020

Conversation

gdbelvin
Copy link
Contributor

@gdbelvin gdbelvin commented Jan 3, 2020

The Kubernetes configs were originally created with kompose convert but the k8 configs have since significantly diverged due to sidecars, local and gke specific configs.

This PR removes kompose metadata that is no longer correct.

The k8 configs and the docker-compose configs are currently maintained by hand because there is not an exact 1:1 feature mapping between them.

@gdbelvin gdbelvin requested a review from a team as a code owner January 3, 2020 18:05
@codecov
Copy link

codecov bot commented Jan 3, 2020

Codecov Report

Merging #1425 into master will increase coverage by 0.09%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1425      +/-   ##
==========================================
+ Coverage   66.31%   66.41%   +0.09%     
==========================================
  Files          54       54              
  Lines        4026     4026              
==========================================
+ Hits         2670     2674       +4     
+ Misses        962      960       -2     
+ Partials      394      392       -2
Impacted Files Coverage Δ
core/sequencer/server.go 73.61% <0%> (+0.65%) ⬆️
core/sequencer/trillian_client.go 61.42% <0%> (+2.85%) ⬆️

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 5c733f7...caf2dc2. Read the comment docs.

Copy link

@NatalieDoduc NatalieDoduc left a comment

Choose a reason for hiding this comment

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

Cleanup is always good. Please see some questions I have about the nature of the changes, though. Thanks!

@@ -1,7 +1,6 @@
apiVersion: v1
kind: Pod
metadata:
creationTimestamp: null

Choose a reason for hiding this comment

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

what did this do? can you also add a mention about creationTimestamp in the PR description?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know what this does, but from what I read in the k8 documentation, empty and default fields in the configs do no need be be specified.

Choose a reason for hiding this comment

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

Could you amend the PR description to mention this change as well?

@@ -1,10 +1,6 @@
apiVersion: apps/v1
kind: Deployment
metadata:
annotations:
kompose.cmd: kompose convert --file ../../docker-compose.yml

Choose a reason for hiding this comment

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

I'm guessing this was added as a hint to how the .yaml file was created? If that's the case, it seems mildly useful to keep (if possibly outdated, from the looks of the path) Maybe add this information as part of ./deploy/kubernetes/README.md if maintaining these annotations proves too cumbersome,

Just curious what the log bloating you refer to is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the original command that generated these files, but the two configs have now significantly diverged to the point that it is no longer useful to document this.

Choose a reason for hiding this comment

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

Ah ha - so these files have now been manually changed, since their original kompose convert creation. Makes sense to not document in that case. One question, though, what is the relationship if any between the docker-compose file and the kube files, then? Why do we use one over the other?

Copy link

@NatalieDoduc NatalieDoduc left a comment

Choose a reason for hiding this comment

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

lgtm, but can you please add information on how/when to update the docker-compose+kube files and vice versa. thanks!

@gdbelvin gdbelvin merged commit 214c0cf into google:master Jan 6, 2020
@gdbelvin gdbelvin deleted the kompose branch January 6, 2020 12:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants