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

Loki: Fix version info issue that shows wrong version (#7669, #8055) #8232

Merged
merged 1 commit into from
Jan 25, 2023

Conversation

HwangTaehyun
Copy link
Contributor

@HwangTaehyun HwangTaehyun commented Jan 22, 2023

What this PR does / why we need it:

  • It should fetch all tags before extracting tag info to ldflag of the build.

    • This issue could happen if a build tag like v2.7.1 was not fetched properly and then the build process was running.

    • The following promtail is https://github.com/grafana/loki/releases/download/v2.7.1/promtail-darwin-arm64.zip

      image
    • If there's no tag on the exact commit to publish, then BRANCH variable becomes HEAD, so the VERSION variable becomes like "HEAD-e0af1cc".

      • loki/tools/image-tag

        Lines 7 to 21 in 86dcc82

        WIP=$(git diff --quiet || echo '-WIP')
        BRANCH=$(git rev-parse --abbrev-ref HEAD | sed 's#/#-#g')
        # When 7 chars are not enough to be unique, git automatically uses more.
        # We are forcing to 7 here, as we are doing for grafana/grafana as well.
        SHA=$(git rev-parse --short=7 HEAD | head -c7)
        # If not a tag, use branch-hash else use tag
        TAG=$((git describe --exact-match 2> /dev/null || echo "") | sed 's/v//g')
        if [ -z "$TAG" ]
        then
        echo "${BRANCH}"-"${SHA}""${WIP}"
        else
        echo "${TAG}"
        fi
      • loki/Makefile

        Line 35 in 86dcc82

        IMAGE_TAG := $(shell ./tools/image-tag)

Which issue(s) this PR fixes:

Special notes for your reviewer:

  • This can happen when the version tag was not fetched properly before the build process.

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • CHANGELOG.md updated
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

@HwangTaehyun HwangTaehyun requested a review from a team as a code owner January 22, 2023 11:42
@CLAassistant
Copy link

CLAassistant commented Jan 22, 2023

CLA assistant check
All committers have signed the CLA.

@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

@HwangTaehyun HwangTaehyun changed the title Loki: Fix version info issue that shows wrong version (#7699, #8055) Loki: Fix version info issue that shows wrong version (#7669, #8055) Jan 22, 2023
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. Thanks for this contribution

@MichelHollands
Copy link
Contributor

MichelHollands commented Jan 25, 2023

@HwangTaehyun Thanks for this contribution. Can you rebase and make sure the script has the executable bit set?

@HwangTaehyun
Copy link
Contributor Author

@MichelHollands Yeah, I rebased and pushed now. And I already set the executable bit to the fetch-tags script!

@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

@MichelHollands MichelHollands merged commit b2ea121 into grafana:main Jan 25, 2023
@chaudum
Copy link
Contributor

chaudum commented Jan 27, 2023

@MichelHollands Can you make sure this change is backported to the release-2.7.x branch. Unfortunately it's too late for the 2.7.2 release (which happened yesterday), but there may be another 2.7.x release, who knows.

@MichelHollands MichelHollands added the backport release-2.7.x add to a PR to backport it into release 2.7.x label Jan 27, 2023
@grafanabot
Copy link
Collaborator

Hello @MichelHollands!
Backport pull requests need to be either:

  • Pull requests which address bugs,
  • Urgent fixes which need product approval, in order to get merged,
  • Docs changes.

Please, if the current pull request addresses a bug fix, label it with the type/bug label.
If it already has the product approval, please add the product-approved label. For docs changes, please add the type/docs label.
If none of the above applies, please consider removing the backport label and target the next major/minor release.
Thanks!

@MichelHollands MichelHollands added the type/bug Somehing is not working as expected label Jan 27, 2023
@HwangTaehyun
Copy link
Contributor Author

@chaudum @MichelHollands Can I know precisely how publishing works is done? Actually, it does not happen if the publisher checkouts the release tag (like 2.7.1) and runs the build script. However, it seems that the publisher checkout to release commit and runs the build script.

@MichelHollands MichelHollands added backport release-2.7.x add to a PR to backport it into release 2.7.x type/bug Somehing is not working as expected and removed backport release-2.7.x add to a PR to backport it into release 2.7.x type/bug Somehing is not working as expected labels Jan 27, 2023
grafanabot pushed a commit that referenced this pull request Jan 27, 2023
@chaudum
Copy link
Contributor

chaudum commented Jan 27, 2023

@chaudum @MichelHollands Can I know precisely how publishing works is done? Actually, it does not happen if the publisher checkouts the release tag (like 2.7.1) and runs the build script. However, it seems that the publisher checkout to release commit and runs the build script.

The building and publishing of the binaries happens as a DroneCI step that is triggered upon creating a tag.
Here is an example from the run triggered by the 2.7.2 release.
The pipeline stage does check out the tag, but it did not fetch all tags. It worked for the Docker image build steps, because they call git fetch --tags explicitly, see https://github.com/grafana/loki/blob/main/.drone/drone.yml#L289-L294

@HwangTaehyun
Copy link
Contributor Author

@chaudum lol, That's what I want to see! Thank you!

chaudum added a commit that referenced this pull request Mar 21, 2023
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 added a commit that referenced this pull request Mar 21, 2023
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 added a commit that referenced this pull request 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.

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
chaudum added a commit that referenced this pull request Jul 14, 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.

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
MichelHollands pushed a commit that referenced this pull request Jul 14, 2023
**This is a backport of #8854**

---

**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.

**What this PR does / why we need it**:

**Which issue(s) this PR fixes**:
Fixes #<issue number>

**Special notes for your reviewer**:

**Checklist**
- [ ] Reviewed the
[`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md)
guide (**required**)
- [ ] Documentation added
- [ ] Tests updated
- [ ] `CHANGELOG.md` updated
- [ ] If the change is worth mentioning in the release notes, add
`add-to-release-notes` label
- [ ] Changes that require user attention or interaction to upgrade are
documented in `docs/sources/upgrading/_index.md`
- [ ] For Helm chart changes bump the Helm chart version in
`production/helm/loki/Chart.yaml` and update
`production/helm/loki/CHANGELOG.md` and
`production/helm/loki/README.md`. [Example
PR](d10549e)

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport release-2.7.x add to a PR to backport it into release 2.7.x size/XS type/bug Somehing is not working as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

loki 2.7 wrong version information
5 participants