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

Adding basic etcd capability to system #854

Merged
merged 61 commits into from
Apr 12, 2024
Merged

Conversation

grafnu
Copy link
Collaborator

@grafnu grafnu commented Apr 11, 2024

No description provided.

@grafnu grafnu requested a review from azenla April 11, 2024 17:45
Copy link
Collaborator

@azenla azenla left a comment

Choose a reason for hiding this comment

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

LGTM, I made some suggestions that you might want to look at.

@@ -35,8 +35,10 @@
<orderEntry type="library" name="Gradle: commons-cli:commons-cli:1.5.0" level="project" />
<orderEntry type="library" name="Gradle: com.google.cloud:google-cloud-logging:3.14.9" level="project" />
<orderEntry type="library" name="Gradle: io.github.clearblade:clearblade-cloud-iot:1.0.5" level="project" />
<orderEntry type="library" name="Gradle: io.etcd:jetcd-core:0.6.1" level="project" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Out of curiosity, is it intended that IntelliJ files are committed? In general I recommend excluding the IntellIJ configs, because they can be regenerated from Gradle, and it tends to clog up the repository.

It's not a huge problem, but it might be cleaner to deleted them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes -- I generally follow the IDEA guidelines on this... include everything except the workplace files. They've sequestered the "individual specific" stuff in one place. I've never seen it actually clog up the repository... (yes, it's redundant but mostly the tools don't care and the churn is pretty low". If it's causing a problem somewhere let me know and we can address (sometimes personal paths creep their ways in), but ultimately it's generally useful for sharing some things like run/debug configurations etc...

spec:
containers:
- name: etcd-set
image: quay.io/coreos/etcd:latest
Copy link
Collaborator

Choose a reason for hiding this comment

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

consider pinning this to a specific version, such as the one set above. In general, for security and compatibility reasons it is recommended to pin container images to a specific version. Observability tools and such can provide information that the container is out of date.

For the CoreOS etcd image it may not be a big deal, but if you were to say, pull a container image from a random outsider, they could attack any infra by pushing new versions to latest.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure -- it's what I usually do, this was just cut-and-paste stuff I wasn't paying attention to... Usually I find it's not security or compatibility reasons, but rather stability and consistency -- without a pinned version things can randomly change and randomly introduce bugs that are then really confusing as to why things suddenly started to behave differently.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well then. It turns out that the "latest" tag is actually 6 years old, and that the more recent versions don't have a shell in them... it's possible, but would require rejiggering this a bit to get the initial etcd startup to work -- not something I would like to tackle just now. So, gonna put it on "the list" of things to do. Likely need to setup a config-map for the cmdline parameters is all. This of course will all come back to haunt me horribly when they do change the "latest" tag for some reason and everything suddenly decides to break...

@grafnu grafnu merged commit fe62bcf into faucetsdn:master Apr 12, 2024
9 checks passed
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.

2 participants