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

chore: update the way the LTS images are built #8953

Merged
merged 2 commits into from
Jul 18, 2023

Conversation

plumpy
Copy link
Collaborator

@plumpy plumpy commented Jul 17, 2023

I moved everything from Dockerfile.deps.lts into Dockerfile.lts directly. Unforunately, this makes this diff pretty useless, but basically the only changes were the download-skaffold section at the top and the removal of the Go installation (the last two lines).

@plumpy
Copy link
Collaborator Author

plumpy commented Jul 17, 2023

FYI, proposing this on release/v2.6 first. Then I'll build a new release and if it all seems to work, I'll cherrypick it to main and the other LTS branches.

@codecov
Copy link

codecov bot commented Jul 17, 2023

Codecov Report

❗ No coverage uploaded for pull request base (release/v2.6@22825cb). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff               @@
##             release/v2.6    #8953   +/-   ##
===============================================
  Coverage                ?   63.62%           
===============================================
  Files                   ?      624           
  Lines                   ?    31925           
  Branches                ?        0           
===============================================
  Hits                    ?    20311           
  Misses                  ?    10087           
  Partials                ?     1527           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@ericzzzzzzz ericzzzzzzz left a comment

Choose a reason for hiding this comment

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

Maybe test it with a person project by connecting gcb with your fork ?

args:
- 'build'
- '--cache-from'
- 'gcr.io/$PROJECT_ID/skaffold-builder:latest'
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 need to add

 - '--build-arg'
 - 'SKAFFOLD_VERSION=$TAG_NAME'

to inject version to the build context

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, sorry, I was testing this with a modified version of this file (mostly because I didn't want to push to AR too) and then I forgot to copy my fixes back into this file. Fixed!

- 'gcr.io/$PROJECT_ID/skaffold-builder:latest'
- 'gcr.io/$PROJECT_ID/skaffold:$_SCANNING_MARKER-lts'
- '-t'
- 'us-east1-docker.pkg.dev/$PROJECT_ID/scanning/skaffold:$TAG_NAME-lts'
Copy link
Contributor

Choose a reason for hiding this comment

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

This means, artifact registry scanner will scan all binary deps, probably we want to do it anyway. Just double check this is intended behavior?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh.. seems that our internal scanner is already doing this may be we don't need to push the image to ar anymore

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, I removed AR. I wasn't sure why that was there but didn't want to change it...

@plumpy
Copy link
Collaborator Author

plumpy commented Jul 17, 2023

Maybe test it with a person project by connecting gcb with your fork ?

I testing it by doing this:
gcloud builds submit --project=$MYPROJECT --substitutions=TAG_NAME=v2.6.1,_SCANNING_MARKER=public-image-2.6 --config=deploy/cloudbuild-lts.yaml

Do you think that's good enough or should I wire up a trigger?

@plumpy plumpy merged commit 0d544d2 into GoogleContainerTools:release/v2.6 Jul 18, 2023
13 checks passed
@plumpy plumpy deleted the simplify_lts branch July 18, 2023 19:56
plumpy added a commit to plumpy/skaffold that referenced this pull request Jul 19, 2023
…8953)

* chore: update the way the LTS images are built

* Add the --build-arg and remove the Artifact Registry copy.

(cherry picked from commit 0d544d2)
plumpy added a commit to plumpy/skaffold that referenced this pull request Jul 19, 2023
…8953)

* chore: update the way the LTS images are built

* Add the --build-arg and remove the Artifact Registry copy.
plumpy added a commit to plumpy/skaffold that referenced this pull request Jul 19, 2023
…8953)

* chore: update the way the LTS images are built

* Add the --build-arg and remove the Artifact Registry copy.
plumpy added a commit to plumpy/skaffold that referenced this pull request Jul 19, 2023
…8953)

* chore: update the way the LTS images are built

* Add the --build-arg and remove the Artifact Registry copy.
plumpy added a commit that referenced this pull request Jul 19, 2023
* chore: update the way the LTS images are built

* Add the --build-arg and remove the Artifact Registry copy.

(cherry picked from commit 0d544d2)
plumpy added a commit that referenced this pull request Jul 19, 2023
* chore: update the way the LTS images are built

* Add the --build-arg and remove the Artifact Registry copy.
plumpy added a commit that referenced this pull request Jul 19, 2023
* chore: update the way the LTS images are built

* Add the --build-arg and remove the Artifact Registry copy.
plumpy added a commit that referenced this pull request Jul 19, 2023
* chore: update the way the LTS images are built

* Add the --build-arg and remove the Artifact Registry copy.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants