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

PMM-11261 Update PMM via watchtower #2844

Merged
merged 20 commits into from
Apr 18, 2024
Merged

Conversation

BupycHuk
Copy link
Member

PMM-11261

Link to the Feature Build: SUBMODULES-3562

If this PR adds or removes or alters one or more API endpoints, please review and add or update the relevant API documents as well:

  • API Docs updated

If this PR is related to some other PRs in this or other repositories, please provide links to those PRs:

  • Links to related pull requests (optional).

@BupycHuk BupycHuk requested a review from a team as a code owner February 26, 2024 11:09
@BupycHuk BupycHuk requested review from idoqo, JiriCtvrtka and ademidoff and removed request for a team February 26, 2024 11:09
Copy link

codecov bot commented Feb 26, 2024

Codecov Report

Attention: Patch coverage is 0.80645% with 123 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (v3@c43ab4e). Click here to learn what that means.

❗ Current head 4104986 differs from pull request most recent head 098a920. Consider uploading reports for the commit 098a920 to get more accurate results

Files Patch % Lines
managed/services/server/updater.go 0.00% 111 Missing ⚠️
managed/services/server/server.go 12.50% 7 Missing ⚠️
managed/cmd/pmm-managed/main.go 0.00% 5 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##             v3    #2844   +/-   ##
=====================================
  Coverage      ?   43.31%           
=====================================
  Files         ?      365           
  Lines         ?    42707           
  Branches      ?        0           
=====================================
  Hits          ?    18498           
  Misses        ?    22601           
  Partials      ?     1608           
Flag Coverage Δ
agent 52.59% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -969,6 +969,20 @@ func main() { //nolint:cyclop,maintidx

dumpService := dump.New(db)

// Get the hostname from the environment variable
watchtowerHostname := os.Getenv("PMM_WATCHTOWER_HOSTNAME")
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Let's keep in mind that, host would contain the port, while hostname wouldn't.

Copy link
Member

Choose a reason for hiding this comment

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

Just to provide evidence :)

Screenshot 2024-02-26 at 15 38 58

Comment on lines 974 to 976
if watchtowerHostname == "" {
watchtowerHostname = "http://watchtower:8080"
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if watchtowerHostname == "" {
watchtowerHostname = "http://watchtower:8080"
}
if watchtowerHost == "" {
watchtowerHost = "http://watchtower:8080"
}

managed/services/server/server.go Outdated Show resolved Hide resolved
managed/services/server/updater.go Outdated Show resolved Hide resolved
managed/services/server/updater.go Outdated Show resolved Hide resolved
version/update.go Outdated Show resolved Hide resolved
managed/services/server/updater.go Outdated Show resolved Hide resolved
managed/services/server/updater.go Outdated Show resolved Hide resolved
managed/services/server/updater.go Show resolved Hide resolved
managed/services/server/updater.go Show resolved Hide resolved
managed/services/server/updater.go Outdated Show resolved Hide resolved
managed/services/server/updater.go Outdated Show resolved Hide resolved
managed/services/server/updater.go Outdated Show resolved Hide resolved
managed/services/server/updater.go Outdated Show resolved Hide resolved
version/update.go Outdated Show resolved Hide resolved
managed/services/server/updater.go Outdated Show resolved Hide resolved
managed/services/server/updater.go Show resolved Hide resolved
managed/services/server/updater.go Outdated Show resolved Hide resolved
managed/services/server/updater.go Show resolved Hide resolved
managed/services/server/updater.go Show resolved Hide resolved
managed/services/server/updater.go Outdated Show resolved Hide resolved
managed/services/server/updater.go Outdated Show resolved Hide resolved
@BupycHuk BupycHuk requested a review from ademidoff March 13, 2024 10:59
docker-compose.yml Outdated Show resolved Hide resolved
managed/cmd/pmm-managed/main.go Outdated Show resolved Hide resolved
managed/cmd/pmm-managed/main.go Outdated Show resolved Hide resolved
managed/cmd/pmm-managed/main.go Outdated Show resolved Hide resolved
managed/services/server/updater.go Outdated Show resolved Hide resolved
@BupycHuk BupycHuk requested a review from ademidoff March 21, 2024 12:37
docker-compose.yml Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
case err != nil && !os.IsNotExist(err):
s.l.WithError(err).Error("Failed to read file")
return nil, errors.Wrap(err, "failed to read file")
case os.Getenv("PMM_SERVER_UPDATE_VERSION") != "":
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if this shouldn't be an if statement, outside of the switch? It does not seem to depend on the result of os.ReadFile, does it?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's not about depending on the result of readfile, but more about priorities. I can rewrite it to ifs instead of switch-case

managed/services/server/updater.go Outdated Show resolved Hide resolved
managed/services/server/updater.go Outdated Show resolved Hide resolved
@BupycHuk BupycHuk merged commit 38f2862 into v3 Apr 18, 2024
24 checks passed
@BupycHuk BupycHuk deleted the PMM-11261-update-via-watchtower branch April 18, 2024 15:54
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.

4 participants