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: use context - seems like it fixes the ldflags issue #65

Merged
merged 1 commit into from
Jul 10, 2023
Merged

feat: use context - seems like it fixes the ldflags issue #65

merged 1 commit into from
Jul 10, 2023

Conversation

tty47
Copy link
Contributor

@tty47 tty47 commented Jul 10, 2023

hello team!

I've added the property context to the pipelines due to the issue that we're facing that we lose the ldflags values after building the container using GitHub.

I've done some different tests, and I realized that if we added this property, we would keep the values and won't need to add the embed files in the repos.

Some of the containers tested are:
https://github.com/jrmanes/celestia-node/pkgs/container/celestia-node
https://github.com/jrmanes/.github/commits/main/.github/workflows/reusable_dockerfile_pipeline.yml


Some images:

Celestia-node:

Without using context during build:

Screenshot 2023-07-10 at 12 38 53

Using context during build:
Screenshot 2023-07-10 at 12 51 36


Celestia-app:

It will report only if it's in a tag, it's okay if we cannot see it, only with tags


I hope this will fix the issue that we're facing atm

Thanks in advance!

Jose Ramon Mañes

Signed-off-by: Jose Ramon Mañes <jose@celestia.org>
@tty47 tty47 self-assigned this Jul 10, 2023
@tty47 tty47 requested review from MSevey and a team as code owners July 10, 2023 10:54
@tty47 tty47 added the bug Something isn't working label Jul 10, 2023
@tty47 tty47 requested review from sysrex, Bidon15 and smuu July 10, 2023 10:55
@tty47 tty47 enabled auto-merge (squash) July 10, 2023 12:36
Copy link

@sysrex sysrex left a comment

Choose a reason for hiding this comment

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

awesome find

@tty47 tty47 merged commit 911f77b into celestiaorg:main Jul 10, 2023
7 checks passed
github-merge-queue bot pushed a commit to celestiaorg/celestia-node that referenced this pull request Jul 12, 2023
hello team!

I've created a new version in the common pipeline, this one should fix
the issues with the forks and we'll have the version output that we're
currently missing when using the docker containers.

If this works, we could close the PR:
#2421
I've tested on my end and seems to be working 🤞 

*PR where we've done the fix*:
celestiaorg/.github#65

Thanks in advance! 🚀 


Jose Ramon Mañes


celestiaorg/devops#370

---------

Signed-off-by: Jose Ramon Mañes <jose@celestia.org>
Wondertan pushed a commit to celestiaorg/celestia-node that referenced this pull request Jul 12, 2023
hello team!

I've created a new version in the common pipeline, this one should fix the issues with the forks and we'll have the version output that we're currently missing when using the docker containers.

If this works, we could close the PR: #2421
I've tested on my end and seems to be working crossed_fingers

PR where we've done the fix: celestiaorg/.github#65

Thanks in advance! rocket

Jose Ramon Mañes

celestiaorg/devops#370
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants