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

Updated Dockerfile to use the latest Go version 1.22 and added Docker… #292

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

anishsajikumar-devsecops

… build and push pipeline

1. Overview

2. Implementation details

3. How to test/use

4. Checklist

  • Does the Readme need to be updated?

5. Limitations (optional)

6. Future Work (optional)

Copy link
Contributor

@xlab xlab left a comment

Choose a reason for hiding this comment

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

The workflow there is different from one that I proposed to use as the reference
https://github.com/noble-assets/noble/blob/main/.github/workflows/docker-publish.yaml

If there are any obvious reasons that prevent us from using the same approach, we can discuss those.

@@ -1,4 +1,4 @@
FROM golang:1.21.5-alpine3.18 AS go-builder
FROM golang:1.22.0-alpine3.18 AS go-builder
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't bump Go version, I wrote that we keep on golang:1.21.x until it's upgraded in heighliner there
https://github.com/strangelove-ventures/heighliner/blob/c88e63a992c9cb02c0089e7074acb19248f6fbc7/builder/go_versions.go#L15

on:
push:
branches:
- 'main'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we shouldn't push images on docker hub that are not associated with a release.

platforms: linux/amd64,linux/arm64
tags: ${{ env.IMAGE_NAME }}:${{ env.LATEST_VERSION }}
push: true
build-args: CONFIG_PATH=./chains.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, is it using heighliner?

README.md Outdated
@@ -1,143 +1,94 @@
# persistenceCore
![Persistence Core](https://www.asiacryptotoday.com/wp-content/uploads/2020/09/persistence-protocol.jpg)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the README in a separate PR. Changing CI pipeline for Docker is a functional concern, the README text is non-functional concern, we should separate such things in different PRs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, and don't reference JPEGs from 3rd party websites please in any file of the official codebase

@ajeet97
Copy link
Contributor

ajeet97 commented Mar 30, 2024

Heighliner image CI added here - #313

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.

3 participants