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.

Sign up for GitHub

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

Jump to bottom

fix: api server should use ARGOCD_SERVER_LOG_LEVEL instead of ARGOCD_REPO_SERVER_LOGLEVEL (#9970) #9975

Merged
merged 4 commits into from Jul 14, 2022
Merged

fix: api server should use ARGOCD_SERVER_LOG_LEVEL instead of ARGOCD_REPO_SERVER_LOGLEVEL (#9970) #9975

merged 4 commits into from Jul 14, 2022

Conversation

usernameisnull
Copy link
Contributor

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • Optional. My organization is added to USERS.md.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).
    Fixes ArgoCD Server Log Level env var incorrect #9970

@codecov
Copy link

codecov bot commented Jul 13, 2022 •
edited

Codecov Report

Merging #9975 (adab3ec) into master (dee67bd) will decrease coverage by 0.03%.
The diff coverage is 53.84%.

@@            Coverage Diff             @@
##           master    #9975      +/-   ##
==========================================
- Coverage   46.00%   45.96%   -0.04%     
==========================================
  Files         227      227              
  Lines       27236    27276      +40     
==========================================
+ Hits        12529    12538       +9     
- Misses      13009    13036      +27     
- Partials     1698     1702       +4
Impacted Files Coverage Δ
util/dex/dex.go 57.14% <16.66%> (-30.74%) ⬇️
util/tls/tls.go 74.66% <23.07%> (-3.54%) ⬇️
util/dex/config.go 85.56% <50.00%> (-3.45%) ⬇️
server/server.go 51.49% <60.00%> (+0.24%) ⬆️
util/oidc/oidc.go 56.12% <100.00%> (+0.34%) ⬆️
util/session/sessionmanager.go 73.94% <100.00%> (+0.20%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1be1a04...adab3ec. Read the comment docs.

crenshaw-dev
crenshaw-dev requested changes Jul 13, 2022
View reviewed changes
Copy link
Collaborator

@crenshaw-dev crenshaw-dev left a comment

Choose a reason for hiding this comment

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

Changed a little bit too much. :-) But after those changes are reverted, the remaining changes are correct!

manifests/base/repo-server/argocd-repo-server-deployment.yaml Outdated
@@ -35,7 +35,7 @@ spec:
name: argocd-cmd-params-cm
key: reposerver.log.format
optional: true
- name: ARGOCD_REPO_SERVER_LOGLEVEL
- name: ARGOCD_SERVER_LOG_LEVEL
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should not be changed here, since it is the repo-server deployment.

cmd/argocd-repo-server/commands/argocd_repo_server.go Outdated
Comment on lines 195 to 198
cmdutil.LogFormat = os.Getenv("ARGOCD_SERVER_LOG_LEVEL")
}
command.Flags().StringVar(&cmdutil.LogFormat, "logformat", env.StringFromEnv("ARGOCD_REPO_SERVER_LOGFORMAT", "text"), "Set the logging format. One of: text|json")
command.Flags().StringVar(&cmdutil.LogLevel, "loglevel", env.StringFromEnv("ARGOCD_REPO_SERVER_LOGLEVEL", "info"), "Set the logging level. One of: debug|info|warn|error")
command.Flags().StringVar(&cmdutil.LogLevel, "loglevel", env.StringFromEnv("ARGOCD_SERVER_LOG_LEVEL", "info"), "Set the logging level. One of: debug|info|warn|error")
Copy link
Collaborator

Choose a reason for hiding this comment

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

These should not be changed, since this is the repo server command code.

mabing added 2 commits July 14, 2022 01:25
change ARGOCD_REPO_SERVER_LOGLEVEL to ARGOCD_SERVER_LOG_LEVEL
e303c4c 
Signed-off-by: mabing <bing.ma@daocloud.io>
Changed too much,revert it.
c38fdaa 
Signed-off-by: mabing <bing.ma@daocloud.io>
crenshaw-dev
crenshaw-dev reviewed Jul 13, 2022
View reviewed changes
manifests/ha/install.yaml Outdated
@@ -10792,7 +10792,7 @@ spec:
key: reposerver.log.format
name: argocd-cmd-params-cm
optional: true
- name: ARGOCD_REPO_SERVER_LOGLEVEL
- name: ARGOCD_SERVER_LOG_LEVEL
Copy link
Collaborator

Choose a reason for hiding this comment

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

You might have to run make manifests or make manifests-local to clear this up. :-)

mabing and others added 2 commits July 14, 2022 23:01
make manifests
ac0d419 
Signed-off-by: mabing <bing.ma@daocloud.io>
@usernameisnull
Merge branch 'argoproj:master' into master
adab3ec
@crenshaw-dev crenshaw-dev changed the title change ARGOCD_REPO_SERVER_LOGLEVEL to ARGOCD_SERVER_LOG_LEVEL fix: change ARGOCD_REPO_SERVER_LOGLEVEL to ARGOCD_SERVER_LOG_LEVEL Jul 14, 2022
@crenshaw-dev crenshaw-dev changed the title fix: change ARGOCD_REPO_SERVER_LOGLEVEL to ARGOCD_SERVER_LOG_LEVEL fix: change ARGOCD_REPO_SERVER_LOGLEVEL to ARGOCD_SERVER_LOG_LEVEL (#9970) Jul 14, 2022
notfromstatefarm
notfromstatefarm approved these changes Jul 14, 2022
View reviewed changes
Copy link
Contributor

@notfromstatefarm notfromstatefarm left a comment

Choose a reason for hiding this comment

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

LGTM. @crenshaw-dev regarding your concern on Slack, I think that if someone dropped the default 'env' block for a hard-coded value, they did so knowing the consequences for their upgrade process. Besides that, in most cases you only change the log level temporarily.

I also recommend changing the PR title to fix: api server should use ARGOCD_SERVER_LOG_LEVEL instead of ARGOCD_REPO_SERVER_LOGLEVEL. Otherwise it may be unclear in the release notes.

@crenshaw-dev crenshaw-dev changed the title fix: change ARGOCD_REPO_SERVER_LOGLEVEL to ARGOCD_SERVER_LOG_LEVEL (#9970) fix: api server should use ARGOCD_SERVER_LOG_LEVEL instead of ARGOCD_REPO_SERVER_LOGLEVEL (#9970) Jul 14, 2022
crenshaw-dev
crenshaw-dev approved these changes Jul 14, 2022
View reviewed changes
Copy link
Collaborator

@crenshaw-dev crenshaw-dev left a comment

Choose a reason for hiding this comment

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

thanks again @usernameisnull !

@crenshaw-dev crenshaw-dev merged commit 7d31d61 into argoproj:master Jul 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crenshaw-dev crenshaw-dev crenshaw-dev approved these changes

@notfromstatefarm notfromstatefarm notfromstatefarm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ArgoCD Server Log Level env var incorrect
3 participants
@usernameisnull @crenshaw-dev @notfromstatefarm