rename confusing properties/flags #82
Conversation
…retaining backwards compatibility
| @@ -15,13 +15,13 @@ Please keep chains in alphabetical order. | |||
|
|
|||
| `github-repo` -> The repo name of the location of the chain binary. | |||
|
|
|||
| `language` -> Used in the Docker `FROM` argument to create a a docker base image. OPTIONS: "go, rust, nix, imported". Use "imported" if you are not able to build the chain binary from source and are importing a pre-made docker container. | |||
| `dockerfile` -> Which dockerfile strategy to use (folder names under dockerfile/). OPTIONS: `cosmos`, `cargo`, `imported`, or `none`. Use `imported` if you are importing an existing public docker image as a base for the heighliner image. Use `none` if you are not able to build the chain binary from source and need to download binaries into the image instead. | |||
There was a problem hiding this comment.
What's the purpose and differences between Dockerfile, native.Dockerfile, local.Dockerfile? Is there any special handling to keep them all in sync?
There was a problem hiding this comment.
Dockerfileis for buildkit. it is not compatible with standard docker build due toFROM --platform=$BUILDPLATFORMwhich is necessary in buildkit builds to cross compile from an image on the machine's arch. This is the "production" use dockerfile.native.Dockerfileis for non-buildkit builds on your local machine, without the-bflag. It will fetch the chain source from the remote git repository similar to theDockerfile. This is used for building an local image from git source, which is good for things like a local test before making a PR for a chains.yaml addition, or debugginginterchaintestcases.local.Dockerfileis used when the--localflag is passed. This will use the current working directory for the chain source code and is mainly used for chain development flows as it does not require changes to be committed and pushed to the remote git repository to build
A contributor doc would be helpful to explain these differences to keep them in sync, as most changes should be made to all 3.
There was a problem hiding this comment.
I think all of that in the README would be excellent.
cmd/queue.go
Outdated
| basicAuthUser := os.Getenv("GITHUB_USER") | ||
| basicAuthPassword := os.Getenv("GITHUB_PASSWORD") |
There was a problem hiding this comment.
Should these env vars be surfaced in documentation somewhere?
There was a problem hiding this comment.
These were copy and pasted. I don't think they are used for anything though. I'd rather remove them than document them when they aren't tested. A PAT would be a much better auth mechanism anyway.
Yes, this is a lift and shift operation other than adding the new flags/keys/values, deprecation handling, and improved logging/docs/descriptions. I ran through all of the supported use cases in the README. It would be great to get a test harness added to heighliner, I added an issue for that in #83 |
TLDR; Cleanup refactor, rename confusing config property keys/values and flags while retaining backwards compatibility
chains.yamlconfig changes:languagein the chains.yaml config is confusing. This is used to indicate which dockerfile strategy should be used. We currently have dockerfiles for building cosmos sdk chains (renames covered in 2), cargo rust builds (renames covered in 3), imported docker images (imported), and simple images where we don't build anything but drop binaries/libraries in (none)language: gowould make sense in chains.yaml if that were the scope of the build, but it has become more concise than that. Including the rename from 1,language: gobecomesdockerfile: cosmos. Other permutations such aslanguage: go,language: cosmosanddockerfile: gowill also work, but will log out deprecation warnings. In a future release we can remove the deprecated keys/values.language: rust, similarly to 2, would also make sense if it were broad enough to build every rust project, but this is also not the case as it is not compatible with frameworks likenixand others.dockerfile: cargois more applicable here. Same as 2, a deprecation warning will be logged ifrustis used, but it will still work.CLI flags changes:
--version/-vflag is confusing as it does not well portray that it is both the git short ref used for the git checkout, but also used for deriving the resulting docker image tag. This flag has been renamed to--git-ref/-g.--version/-vcan still be used, but a deprecation warning will be logged. If both flags are used,--git-refwill override.--tag/-tis a new optional flag introduced for overriding the resulting docker image tag. If not provided, the image tag will still be derived from the git ref.Organizational changes:
HeighlinerBuilderintroduced, and associated logic moved out ofcmd/build.go. Code organized in build/ and cmd/Log/flag changes:
--git-refflag description updated to reflect that it is the git short ref (branch/tag)--platformflag description updated to indicate that it only applies when using buildkit