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

Adds support for property origin tracking in config server/client. #870

Merged
merged 11 commits into from
Aug 2, 2019

Conversation

spencergibb
Copy link
Member

@spencergibb spencergibb commented Dec 15, 2017

Adds a new format if requested using v2 Accept header, otherwise the
format is backwards compatible so older clients will work with new
servers.

fixes gh-866

/cc @twicksell

requires spring-cloud/spring-cloud-commons#583

@codecov-io
Copy link

codecov-io commented Dec 15, 2017

Codecov Report

Merging #870 into master will decrease coverage by 0.98%.
The diff coverage is 50.51%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #870      +/-   ##
============================================
- Coverage     78.37%   77.38%   -0.99%     
- Complexity     1030     1042      +12     
============================================
  Files           126      129       +3     
  Lines          3699     3776      +77     
  Branches        523      535      +12     
============================================
+ Hits           2899     2922      +23     
- Misses          615      669      +54     
  Partials        185      185
Impacted Files Coverage Δ Complexity Δ
...oud/config/server/resource/ResourceController.java 84.9% <ø> (ø) 20 <0> (ø) ⬇️
...ud/config/environment/PropertyValueDescriptor.java 0% <0%> (ø) 0 <0> (?)
...cloud/config/environment/EnvironmentMediaType.java 0% <0%> (ø) 0 <0> (?)
...ent/EnvironmentEncryptorEnvironmentRepository.java 90.9% <100%> (+0.43%) 9 <4> (+1) ⬆️
...environment/MultipleJGitEnvironmentRepository.java 75.83% <100%> (ø) 35 <0> (ø) ⬇️
...nfig/server/environment/EnvironmentRepository.java 100% <100%> (ø) 1 <1> (?)
...fig/server/config/ConfigServerHealthIndicator.java 82.45% <100%> (ø) 8 <0> (ø) ⬇️
.../config/server/environment/EnvironmentCleaner.java 100% <100%> (ø) 7 <4> (+4) ⬆️
...erver/environment/NativeEnvironmentRepository.java 85% <100%> (+0.1%) 45 <1> (+1) ⬆️
.../environment/AbstractScmEnvironmentRepository.java 90% <100%> (+0.52%) 5 <1> (+1) ⬆️
... and 14 more

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 cbcdcde...881df30. Read the comment docs.

@dsyer
Copy link
Contributor

dsyer commented Dec 18, 2017

It seems like we need this feature, and this looks like a good start, and we clearly have all the data we need. I don't like the format of the JSON though, and it seems like we could maybe even make a more incremental change that didn't require a new media type (or maybe not, but it's worth thinking about). Repeating the file location/name in every property value seems wasteful, and we could use integer fields for the line and column values, to make them more machine readable, and then convert to canonical "Boot" style format in the client.

@spencergibb
Copy link
Member Author

I agree we could probably not repeat the location, but I don't think we can go straight to integers. Ints would work for the file based env repos, like git, but wouldn't work for jdbc or vault. Also, git blame info could go on each origin for git.

@spencergibb
Copy link
Member Author

spencergibb commented Jul 25, 2019

@dsyer @ryanjbaxter @OlgaMaciaszek @marcingrzejszczak @TYsewyn I'm reviving this again.

Two options I can see for the format:

  1. file not repeated (I prefer this I think)
{
    "label": null,
    "name": "foo",
    "profiles": [
        "cloud"
    ],
    "propertySources": [
        {
            "name": "https://github.com/spring-cloud-samples/config-repo/application.yml (document #1)",
            "source": {
                "application.domain": {
                    "origin": "27:11",
                    "value": "${APPLICATION_DOMAIN:cfapps.io}"
                },
                "endpoints.restart": {
                    "origin": "29:12",
                    "value": "enabled"
                }
         }
    ]
}

or repeated

{
    "label": null,
    "name": "foo",
    "profiles": [
        "cloud"
    ],
    "propertySources": [
        {
            "name": "https://github.com/spring-cloud-samples/config-repo/application.yml (document #1)",
            "source": {
                "application.domain": {
                    "origin": "[https://github.com/spring-cloud-samples/config-repo/application.yml]:27:11",
                    "value": "${APPLICATION_DOMAIN:cfapps.io}"
                },
                "endpoints.restart": {
                    "origin": "[https://github.com/spring-cloud-samples/config-repo/application.yml]:29:12",
                    "value": "enabled"
                }
         }
    ]
}

@spencergibb
Copy link
Member Author

Once we get the paired down one (I think I prefer it), we could provide different fields for origin, line and column for origins that support it and an opaque string for others.

@ryanjbaxter
Copy link
Contributor

+1 to the paired down version

@TYsewyn
Copy link
Contributor

TYsewyn commented Jul 26, 2019

I agree on the paired down version, only question that I have now is whether or not we still need to have that (document #1) suffix in the property source its name since we now have the exact location of the property within that file.

@spencergibb
Copy link
Member Author

spencergibb commented Jul 26, 2019

(document #1) comes from spring boot actually

@spencergibb
Copy link
Member Author

Here's a sample client error message

***************************
APPLICATION FAILED TO START
***************************

Description:

Binding to target org.springframework.boot.context.properties.bind.BindException: Failed to bind properties under 'democonfigclient' to com.example.democonfigclient.DemoconfigclientApplication$Config failed:

    Property: democonfigclient.message
    Value: hello from dev profile
    Origin: https://github.com/spring-cloud-samples/config-repo/foo-dev.yml:3:27
    Reason: size must be between 0 and 10

spencergibb and others added 11 commits August 2, 2019 13:40
Adds a new format if requested using v2 Accept header, otherwise the
format is backwards compatible so older clients will work with new
servers.

fixes gh-866
This commit updates URLs to prefer the https protocol. Redirects are not followed to avoid accidentally expanding intentionally shortened URLs (i.e. if using a URL shortener).

# Fixed URLs

## Fixed Success
These URLs were switched to an https URL with a 2xx status. While the status was successful, your review is still recommended.

* [ ] http://www.apache.org/licenses/ with 1 occurrences migrated to:
  https://www.apache.org/licenses/ ([https](https://www.apache.org/licenses/) result 200).
* [ ] http://www.apache.org/licenses/LICENSE-2.0 with 137 occurrences migrated to:
  https://www.apache.org/licenses/LICENSE-2.0 ([https](https://www.apache.org/licenses/LICENSE-2.0) result 200).
@spencergibb spencergibb merged commit 7f547f8 into master Aug 2, 2019
@spencergibb spencergibb deleted the origin-track branch August 2, 2019 19:00
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.

Add support for propery source origin introduced in boot 2
6 participants