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

fix: Don't overwrite ldflags in justfile builds #171

Merged
merged 1 commit into from
Sep 12, 2024
Merged

fix: Don't overwrite ldflags in justfile builds #171

merged 1 commit into from
Sep 12, 2024

Conversation

jpmcb
Copy link
Member

@jpmcb jpmcb commented Sep 11, 2024

Description

There was a small error that was causing the ldflags to get overwritten (one after another). More or less a go build command error.

This also adds missing build-args for the docker container in the GitHub action. Date and sha might be abit more tricky to get into that action, but for now, the version is good enough

Related Tickets & Documents

Closes: #167 (cc @brandonroberts - I ended up going down a rabbit hole on this one, apologies)

Steps to QA

  1. just build-all
  2. Run ./build/pizza-darwin-arm64 version and see the right version and sha

Tier (staff will fill in)

  • Tier 1
  • Tier 2
  • Tier 3
  • Tier 4

Signed-off-by: John McBride <john@opensauced.pizza>
@jpmcb jpmcb requested a review from a team September 11, 2024 23:28
Copy link
Member

@nickytonline nickytonline left a comment

Choose a reason for hiding this comment

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

Ran just build-all then ./build/pizza-darwin-arm64 version and all good now.

=> [builder 2/6] WORKDIR /app                                                                                 0.7s
 => [builder 3/6] COPY go.* ./                                                                                 0.0s

...

pizza-cli on  fix-ldflags [$?] via 🐳 desktop-linux via 🐹 v1.23.0 took 43s 
❯ ./build/pizza-darwin-arm64 version
Version: dev
Sha: 432207b6b2cbe7fa59ba936c8ab546a9cb9203bd
Built at: 2024-09-11-23:45:33

It did mention this though.

 1 warning found (use docker --debug to expand):
 - SecretsUsedInArgOrEnv: Do not use ARG or ENV instructions for sensitive data (ARG "POSTHOG_PUBLIC_API_KEY") (line 8)

@jpmcb
Copy link
Member Author

jpmcb commented Sep 11, 2024

SecretsUsedInArgOrEnv: Do not use ARG or ENV instructions for sensitive data (ARG "POSTHOG_PUBLIC_API_KEY") (line 8)

I reject that suggestion haha 😂 the posthog public key isn't a secret thankfully, otherwise, yes, we'd want to handle it more securely

@nickytonline
Copy link
Member

SecretsUsedInArgOrEnv: Do not use ARG or ENV instructions for sensitive data (ARG "POSTHOG_PUBLIC_API_KEY") (line 8)

I reject that suggestion haha 😂 the posthog public key isn't a secret thankfully, otherwise, yes, we'd want to handle it more securely

Oh yeah, I guess it has no way of knowing if it's a public key or not. 😅

@jpmcb jpmcb merged commit e024687 into beta Sep 12, 2024
9 checks passed
open-sauced bot pushed a commit that referenced this pull request Sep 12, 2024
## [1.4.1-beta.1](v1.4.0...v1.4.1-beta.1) (2024-09-12)

### 🐛 Bug Fixes

* Don't overwrite ldflags in justfile builds ([#171](#171)) ([e024687](e024687))
open-sauced bot pushed a commit that referenced this pull request Sep 16, 2024
## [1.5.0](v1.4.0...v1.5.0) (2024-09-16)

### 🐛 Bug Fixes

* Config path prefers local dir vs. home dir ([#184](#184)) ([859446a](859446a))
* Don't overwrite ldflags in justfile builds ([#171](#171)) ([e024687](e024687))

### 🍕 Features

* Cut 2.0.0 release ([#193](#193)) ([278a833](278a833))
* pizza generate insight command ([#179](#179)) ([7315a1d](7315a1d))
* Update README docs ([#186](#186)) ([99328aa](99328aa))
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.

Bug: npm i -g pizza is missing build time variables
3 participants