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: adjust env vars so that check-env-var-annotations passes #8574

Merged
merged 31 commits into from
Mar 6, 2024

Conversation

phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Mar 5, 2024

Description

  1. chore: adjust documentation of check-env-var-annotations.
  2. display line numbers in the output of check-env-var-annotatrions. This makes it easy to find the exact line that has the problem.
  3. adjust logic for annotation checks so that it really passes when there are no problems.
  4. add introductionVersion "pre5.0" to all environment variable documentation.
  5. add missing descriptions to environment variable documentation.
  6. adjust some env var descriptions
  7. add check-env-var-annotations to drone CI

This sets introductionVersion to "pre5.0" everywhere as a start. I suggest that we merge this, so that introductionVersion is defined everywhere, then I can separately work on removing "pre" for those env vars that are introduced in 5.0

This supersedes PR #8469

Related Issue

Part of #8434

How Has This Been Tested?

https://drone.owncloud.com/owncloud/ocis/32623/1/6

+ make check-env-var-annotations
.make/check-env-var-annotations.sh
All introductionVersion annotations are valid
All description annotations are valid

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

mmattel
mmattel previously approved these changes Mar 5, 2024
Copy link
Contributor

@mmattel mmattel left a comment

Choose a reason for hiding this comment

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

From what I have read, this all looks ok to me 👍

Following issue found: There are envvars that have been added in 5.0 and are therefore not pre5.0
The list added in 5.0 can be found at https://owncloud.dev/services/general-info/env-var-deltas/4.0.0-5.0.0-added/

@mmattel
Copy link
Contributor

mmattel commented Mar 5, 2024

Needs (imho) backport to stable-5.0

@mmattel mmattel dismissed their stale review March 5, 2024 08:01

found inconsistencies

@phil-davis
Copy link
Contributor Author

From what I have read, this all looks ok to me 👍

Following issue found: There are envvars that have been added in 5.0 and are therefore not pre5.0 The list added in 5.0 can be found at https://owncloud.dev/services/general-info/env-var-deltas/4.0.0-5.0.0-added/

I am aware of that. My suggestion is that we can first merge this to get introductionVersion set everywhere.

The exact env vars that are introduced in 5.0 is a bit complex, due to the lines that have multiple env vars. So that is taking more time to sort out.

@phil-davis phil-davis force-pushed the env-var-introductionVersion branch 2 times, most recently from 6cbdf62 to 11522bd Compare March 5, 2024 08:21
Comment on lines +26 to +30
| services/auth-service/pkg/config/tracing.go | `OCIS_TRACING_ENABLED;AUTH_SERVICE_TRACING_ENABLED` | Activates tracing. | |
| | `OCIS_TRACING_TYPE;AUTH_SERVICE_TRACING_TYPE` | The type of tracing. Defaults to '', which is the same as 'jaeger'. Allowed tracing types are 'jaeger' and '' as of now."` | |
| | `OCIS_TRACING_ENDPOINT;AUTH_SERVICE_TRACING_ENDPOINT` | The endpoint of the tracing agent. | |
| | `OCIS_TRACING_COLLECTOR;AUTH_SERVICE_TRACING_COLLECTOR` | The HTTP endpoint for sending spans directly to a collector, i.e. http://jaeger-collector:14268/api/traces. Only used if the tracing endpoint is unset. | |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: these env vars are actually in tracing.go so I have adjusted the md file.
FYI @mmattel @dragonchaser
If I find other minor things in the md file, I will push those also.

@phil-davis
Copy link
Contributor Author

Things I found:
MICRO_REGISTRY in ocis-pkg/registry/registry.go
MICRO_REGISTRY_AUTH_USERNAME in ocis-pkg/natsjsregistry/registry.go
MICRO_REGISTRY_AUTH_PASSWORD in ocis-pkg/natsjsregistry/registry.go
These are not in a format that will allow specifying introduction version.

OCIS_CACHE_DISABLE_PERSISTENCE;GATEWAY_STAT_CACHE_DISABLE_PERSISTENCE not found
OCIS_CACHE_DISABLE_PERSISTENCE;STORAGE_USERS_STAT_CACHE_DISABLE_PERSISTENCE not found
These are in the 4.0.0-5.0.0-added.md file but I can't find them in the code.

Comment on lines 180 to 189
| | `OCIS_CACHE_AUTH_USERNAME;SETTINGS_CACHE_AUTH_USERNAME` | The username to authenticate with the cache. Only applies when store type 'nats-js-kv' is configured. | |
| | `OCIS_CACHE_AUTH_PASSWORD;SETTINGS_CACHE_AUTH_PASSWORD` | The password to authenticate with the cache. Only applies when store type 'nats-js-kv' is configured. | |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that these are the correct names - they exist in services/settings/pkg/config/config.go

@phil-davis
Copy link
Contributor Author

@mmattel @dragonchaser all done - please look and comment about anything that I have missed or needs changing.

@phil-davis
Copy link
Contributor Author

Needs (imho) backport to stable-5.0

I will do that when this Pr to master is reviewed

@mmattel
Copy link
Contributor

mmattel commented Mar 5, 2024

I will do that when this Pr to master is reviewed

👍 Pls add the following related and merged PR to backporting
#8577 ([docs-only] Fix added envars table) and
#8582 ([docs-only] And more envvars that are added to the table)

@mmattel
Copy link
Contributor

mmattel commented Mar 5, 2024

@phil-davis There was a conflict because #8523 got merged which I solved. That PR contained new envvars relevant for master and later 5.1 BUT...

When doing a backport to stable-5.0 you need to change back in services/web/pkg/config/config.go:

	DeprecatedPath string `yaml:"path" env:"WEB_ASSET_PATH" desc:"Serve ownCloud Web assets from a path on the filesystem instead of the builtin assets." deprecationVersion:"5.1.0" removalVersion:"6.0.0" deprecationInfo:"The WEB_ASSET_PATH is deprecated and will be removed in the future." deprecationReplacement:"Use WEB_ASSET_CORE_PATH instead." introductionVersion:"pre5.0"`
	CorePath       string `yaml:"core_path" env:"WEB_ASSET_CORE_PATH" desc:"Serve ownCloud Web assets from a path on the filesystem instead of the builtin assets." introductionVersion:"5.1"`
	AppsPath       string `yaml:"apps_path" env:"WEB_ASSET_APPS_PATH" desc:"Serve ownCloud Web apps assets from a path on the filesystem instead of the builtin assets." introductionVersion:"5.1"`

to

	Path string `yaml:"path" env:"WEB_ASSET_PATH" desc:"Serve ownCloud Web assets from a path on the filesystem instead of the builtin assets." introductionVersion:"pre5.0"`

Else we get non 5.0 related content into the 5.0 branch.

Copy link
Member

@dragonchaser dragonchaser left a comment

Choose a reason for hiding this comment

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

@phil-davis Please merge & backport to stable-5.0
Plus see the backporting comments from @mmattel above.

@phil-davis
Copy link
Contributor Author

Rebased - CI should pass now, and we can merge.

Copy link
Contributor

@mmattel mmattel left a comment

Choose a reason for hiding this comment

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

Looks good from a docs pov 👍

Copy link

sonarcloud bot commented Mar 6, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
43 New Bugs (required ≤ 0)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@phil-davis
Copy link
Contributor Author

Now SonarCloud does not like the order of items in the yaml in ocis-pkg/shared/shared_types.go
It wants to re-order lines like:

	Level  string `yaml:"level" env:"OCIS_LOG_LEVEL" desc:"The log level. Valid values are: 'panic', 'fatal', 'error', 'warn', 'info', 'debug', 'trace'." introductionVersion:"pre5.0"`

to have the items in alphabetical order.

It complains about 43 lines in that one file, but does not complain about the same format in all the services/x/pkg/config/config.go files.

@phil-davis phil-davis merged commit e053600 into master Mar 6, 2024
2 of 3 checks passed
@delete-merged-branch delete-merged-branch bot deleted the env-var-introductionVersion branch March 6, 2024 12:38
ownclouders pushed a commit that referenced this pull request Mar 6, 2024
chore: adjust env vars so that check-env-var-annotations passes
@phil-davis
Copy link
Contributor Author

Note: backported to stable-5.0 in PR #8583

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.

None yet

4 participants