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

Build: Remove ./tools/fetch-tags invocation from Makefile #8849

Closed
wants to merge 6 commits into from

Conversation

chaudum
Copy link
Contributor

@chaudum chaudum commented Mar 21, 2023

What this PR does / why we need it:

This invocation causes problems when make targets are executed in the build image container (BUILD_IN_CONTAINER=true), e.g. the yacc target, because git tries to fetch tags from origin using ssh, causing error message like these:

Host key verification failed.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
error: Could not fetch origin

This is because local ssh keys are not mounted into the build container.

In order not to break CI builds
(#8232) we need to manually fetch the tags as the first step in release pipelines. This is added to the Drone configuration.

This invocation causes problems when make targets are executed in the
build image container (`BUILD_IN_CONTAINER=true`), e.g. the `yacc`
target, because git tries to fetch tags from `origin` using ssh, causing
error message like these:

```
Host key verification failed.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
error: Could not fetch origin
```

This is because local ssh keys are not mounted into the build container.

In order not to break CI builds
(#8232) we need to manually fetch
the tags as the first step in release pipelines. This is added to the
Drone configuration.

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
@chaudum chaudum requested a review from a team as a code owner March 21, 2023 09:59
Copy link
Contributor

@MichelHollands MichelHollands left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Contributor

@vlad-diachenko vlad-diachenko left a comment

Choose a reason for hiding this comment

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

we can not remove installation steps like : apk add --no-cache bash git

@@ -653,8 +666,6 @@ local manifest_ecr(apps, archs) = pipeline('manifest-ecr') {
RELEASE_TAG_REGEXP: '^([0-9]+\\.[0-9]+\\.[0-9]+)$',
},
commands: [
'apk add --no-cache bash git',
Copy link
Contributor

Choose a reason for hiding this comment

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

i will cause an issue that ./tools/image-tag won't be executed becuse it uses bash

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
…d image

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
…in build image

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
Copy link
Contributor

@vlad-diachenko vlad-diachenko left a comment

Choose a reason for hiding this comment

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

Lgtm

@chaudum
Copy link
Contributor Author

chaudum commented Mar 21, 2023

@vlad-diachenko @MichelHollands @dannykopping Opened a new PR because I don't want to open the can of worms that I did with this PR.

@chaudum chaudum closed this Mar 21, 2023
@chaudum chaudum deleted the chaudum/remove-git-fetch-from-makefile branch March 21, 2023 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants