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

feat(manifest): Allow defining accounts #1240

Merged
merged 9 commits into from
Oct 19, 2023
Merged

feat(manifest): Allow defining accounts #1240

merged 9 commits into from
Oct 19, 2023

Conversation

Laubi
Copy link
Contributor

@Laubi Laubi commented Oct 18, 2023 •
edited
Loading

What this PR does / Why we need it:

This PR allows defining accounts in the manifest.yaml.

To do that, a user must increase the manifest-version to 1.1.

Example manifest:

manifestVersion: 1.1                                # manifestVersion needs to be set to at least 1.1

accounts:                                           # New top level property
- name: other-account                               # name
  accountUUID: c3f50f90-a1e2-4e7b-aadb-f3dea28e2294 # account uuid
  apiUrl:                                           # allow overriding endpoint (optional)
    type: environment
    value: ENV_API_URL
  oAuth:                                            # credentials (only oAuth)
    clientId:
      name: ENV_CLIENT_ID
    clientSecret:
      name: ENV_CLIENT_SECRET
    tokenEndpoint:                                  # (optional)
      type: environment
      value: ENV_TOKEN_URL

Does this PR introduce a user-facing change?

No, not really. Everything is still compatible if using monaco 1.0 manifests.

Other changes included

manifest_test.go is now in the manifest_test package so we can do package tests.

Future work

Some identified refactorings:

  1. The manifest-version has 2 parts (major, minor), while the internal version used has three (major, minor, patch). Printing the version easily is not possible because of that, as we would suddenly print the three-part instead of the two parts on messages. A extra 'manifestversion' struct should be introduced.
  2. The code is mostly inside one file (manifest_loader). It's quite difficult to navigate, and splitting it would make sense.

@Laubi Laubi force-pushed the feat/manifest-loading branch from a4d2d61 to b742e82 Compare October 18, 2023 13:18
Laubi added 3 commits October 18, 2023 15:19
@Laubi
feat: Add account definition for manifest
ca5cb51 
To support accounts, a new `account` struct is introduced containing all important fields.
@Laubi
feat: Extend Manifest definition
0114f98 
This change extends the in-memory definition of the manifest.
@Laubi
refactor: Move manifest_test file to manifest_test package
574ad1e 
This allows us to write proper package unit-tests instead of accidently relying on package-internals.
@Laubi Laubi force-pushed the feat/manifest-loading branch from b742e82 to 646e0ad Compare October 18, 2023 13:19
@github-actions
Copy link

github-actions bot commented Oct 18, 2023 •
edited
Loading

Unit Test Results

       2 files  ±  0     222 suites  ±0   22s ⏱️ +4s
1 516 tests +11  1 516 ✔️ +11  0 💤 ±0  0 ❌ ±0 
3 032 runs  +22  3 032 ✔️ +22  0 💤 ±0  0 ❌ ±0 

Results for commit b10e9bd. ± Comparison against base commit 31b0c6a.

This pull request removes 7 and adds 18 tests. Note that renamed tests count towards both. 
github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/manifest ‑ TestVerifyManifestYAML
github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/manifest ‑ TestVerifyManifestYAML/fails_on_malformed_manifest_version
github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/manifest ‑ TestVerifyManifestYAML/fails_on_missing_environments
github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/manifest ‑ TestVerifyManifestYAML/fails_on_missing_projects
github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/manifest ‑ TestVerifyManifestYAML/fails_on_missing_version
github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/manifest ‑ TestVerifyManifestYAML/fails_on_no_longer_supported_manifest_version
github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/manifest ‑ TestVerifyManifestYAML/fails_on_not_yet_supported_manifest_version

github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/manifest ‑ TestInvalidAccounts
github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/manifest ‑ TestInvalidAccounts/accountUUID_is_invalid
github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/manifest ‑ TestInvalidAccounts/accountUUID_is_missing
github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/manifest ‑ TestInvalidAccounts/name_is_missing
github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/manifest ‑ TestInvalidAccounts/oAuth.id_is_not_set
github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/manifest ‑ TestInvalidAccounts/oAuth.secret_is_not_set
github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/manifest ‑ TestInvalidAccounts/oAuth_is_set
github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/manifest ‑ TestManifestLoading
github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/manifest ‑ TestManifestLoading_AccountsInvalid
github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/manifest ‑ TestManifestLoading_AccountsInvalid/Empty_account_uuid
…

♻️ This comment has been updated with latest results.

@Laubi Laubi marked this pull request as ready for review October 18, 2023 14:16
@Laubi Laubi requested a review from a team as a code owner October 18, 2023 14:16
@Laubi Laubi force-pushed the feat/manifest-loading branch from 1e6bfab to 583e0d5 Compare October 19, 2023 07:09
@Laubi Laubi self-assigned this Oct 19, 2023
UnseenWizzard
UnseenWizzard previously approved these changes Oct 19, 2023
View reviewed changes
Copy link
Contributor

@UnseenWizzard UnseenWizzard left a comment

Choose a reason for hiding this comment

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

Changes look good to me overall 👍 Just a few small comments/suggestions

pkg/manifest/manifest.go Outdated Show resolved Hide resolved
pkg/manifest/manifest_loader.go Outdated Show resolved Hide resolved
pkg/manifest/manifest_loader.go Outdated Show resolved Hide resolved
pkg/manifest/account.go Show resolved Hide resolved
internal/featureflags/temporary.go Show resolved Hide resolved
Laubi added 6 commits October 19, 2023 09:52
@Laubi
feat: Introduce new manifest version 1.1
dd263c5 
The new version allows defining accounts.
If a prior version is used, we print an error describing that accounts are not supported.
Additionally, simplify the manifest version checking
@Laubi
feat: Load accounts in-memory
8868a2c 
This change converts all persisted accounts to in-memory definitions
@Laubi
refactor: Slit account stuff in its own file
e5432e9 
This change allows nice restructuring. Additionally, introduced unit tests for that file, and errors to easily check what happened.
@Laubi
test: Fix manifest loader test
70c2b08 
The test fails as the current code returns an empty map instead of nil.
@Laubi
feat: Only write 1.1 manifests if the feature flag is enabled.
1e89f61 
Also: introduce a feature flag
@Laubi
refactor: Unify naming for reading manifest
b10e9bd 
Instead of having *convert/parse/to*, we now have *parse* as the single prefix for reading manifest related data.
@Laubi Laubi force-pushed the feat/manifest-loading branch from 583e0d5 to b10e9bd Compare October 19, 2023 07:53
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Laubi Laubi merged commit 599bf18 into main Oct 19, 2023
11 checks passed
@Laubi Laubi deleted the feat/manifest-loading branch October 19, 2023 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@UnseenWizzard UnseenWizzard UnseenWizzard approved these changes

Assignees

@Laubi Laubi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Laubi @UnseenWizzard