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

Feat/first nib images #1

Merged
merged 89 commits into from
Dec 23, 2024
Merged

Feat/first nib images #1

merged 89 commits into from
Dec 23, 2024

Conversation

mateipopa
Copy link
Contributor

This is still work in progress, but while there still are things to test, it'll give you an idea of what the approach is.
Please fire away with feedback - it'll be appreciated as usual.

Copy link

@mjr-blockjoy mjr-blockjoy left a comment

Choose a reason for hiding this comment

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

We also need to add protocols.yaml with protocols definition e.g. https://github.com/blockjoy/blockvisor/blob/master/bv/tests/protocols.yaml

- key: erigon-goerli-archive
archive_pointers:
- pointer: !store_id ethereum-erigon-goerli-archive-v1
- pointer: !store_id ethereum-erigon-goerli-archive-v1

Choose a reason for hiding this comment

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

It doesn't make sense to define new_archive: true property if it have the same store_id.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I wanted to achieve here is that whenever you select the tracing API, the dataset should point to the archive store rather than the full one. Can we configure this behavior more explicitly?

GRAFANA_PROM_API_KEY=${{ secrets.GRAFANA_PROM_API_KEY }}
cache-from: type=gha
cache-to: type=gha,mode=max
tags: ghcr.io/blockjoy/${{ steps.version.outputs.image_name }}:${{ steps.version.outputs.image_tag }}
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 should also include a latest tag as we previously discussed. Trying to update all the dependencies with the hash is going to create a lot of work for every image update. Or we omit the hash and stick to the semantic version (i.e. we shouldn't ever have a version 1.1.1-hask8d and a 1.1.1-ja9dk1...two of the same sematic with different hashes)

Copy link
Contributor Author

@mateipopa mateipopa Nov 26, 2024

Choose a reason for hiding this comment

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

Agreed, this mainly stemmed from the fact that I needed a way to bootstrap the pipeline since initially, no images were built that could satisfy the requirements of the next images. I ended up going for the commit of the hash and then updated the dependent Dockerfiles manually. We should only need the semver and only go for full release versioning, so don't allow pre-release versions on prod.

LE: I've moved the rest of my comment to the main PR discussion

}

http://{{ hostname }}{{ tld }}, https://{{ hostname }}{{ tld }}, *.blkjy.io, http:// {
log {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
log {
tls {
dns cloudflare {$CLOUDFLARE_API_KEY}
resolvers 1.1.1.1
}
log {

Copy link
Contributor

Choose a reason for hiding this comment

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

We should include this, this will prevent the nodes from trying to use 127.0.0.1 for dns lookups, which doesn't work and helps gets certs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also there's a typo in the variable syntax

}

http://{{ hostname }}{{ tld }}, https://{{ hostname }}{{ tld }}, *.blkjy.io, http:// {
log {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
log {
tls {
dns cloudflare {$CLOUDFLARE_API_KEY}
resolvers 1.1.1.1
}
log {

@mateipopa mateipopa force-pushed the feat/first-nib-images branch from a2fe66b to ada7d4f Compare December 23, 2024 15:24
@mateipopa mateipopa merged commit 9c578e8 into main Dec 23, 2024
4 checks passed
@mateipopa mateipopa deleted the feat/first-nib-images branch January 22, 2025 13:56
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.

4 participants