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

Update configuration_metadata.go #513

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

AamnaZahid
Copy link

@AamnaZahid AamnaZahid commented Oct 2, 2024

Support added for the configuration-properties.names property.

Summary

Use Cases

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

@AamnaZahid AamnaZahid requested a review from a team as a code owner October 2, 2024 06:34
Copy link

linux-foundation-easycla bot commented Oct 2, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: AamnaZahid / name: Aamna Zahid (00998bd, eb32c22)
  • ✅ login: anthonydahanne / name: Anthony Dahanne (d52c2e3)

@anthonydahanne
Copy link
Member

anthonydahanne commented Oct 2, 2024

Hello @AamnaZahid !
Thanks for your contribution!
Can you please:

  • provide context / explain what those changes will enable?
  • add unit tests
  • sign the CLA

So that we'll be able to review.
Thank you!

@AamnaZahid
Copy link
Author

AamnaZahid commented Oct 4, 2024

Context/Explanation of Changes

Before:
The function only processed the configuration-properties.classes field by splitting its values and appending them to the classes list. It then matched these classes against SourceType values in metadata groups and properties.

After:
In addition to classes, code now handle configuration-properties.names similarly, where the names values are also processed and stored in a names list.
Both classes and names lists are merged into a combined list, which is used to match against the SourceType or Name fields in metadata Groups and Properties.
This ensures that both properties (classes and names) are additive and provide more flexibility for users to define mappings based on either class or name.

What those changes will enable?

  1. Support for configuration-properties.names: Now, users can specify both classes and names in the dataflow-configuration-metadata.properties file. This makes the configuration process more intuitive and documented.
  2. Expanded Mapping Capabilities: The new logic allows for mapping based on both SourceType and Name fields, rather than being restricted to just SourceType. This provides more flexibility in defining and processing configuration metadata.

@AamnaZahid
Copy link
Author

Hi @anthonydahanne
Hope you are good
I added some test cases.
Plz review it now. Thanks

@dmikusa
Copy link
Contributor

dmikusa commented Oct 4, 2024

Support for configuration-properties.names: Now, users can specify both classes and names in the dataflow-configuration-metadata.properties file. This makes the configuration process more intuitive and documented.

OK, interesting. This is what I was trying to figure out as well. Where do these files come from, what apps are depending on them.

Can you provide a doc link that explains these files a little more? I want to make sure that there is some context here so we, as maintainers, know how this feature is being used. That'll prevent regressions in the future. Thanks

@anthonydahanne
Copy link
Member

@anthonydahanne anthonydahanne added semver:patch A change requiring a patch version bump type:enhancement A general enhancement labels Oct 23, 2024
@AamnaZahid
Copy link
Author

AamnaZahid commented Oct 23, 2024 via email

* replace deprecated calls to os.ReadFile/WriteFile
* fix "handles empty names gracefully" test: if a property did not make it into dataflow-configuration-metadata.properties then it's not considered, even if it was found in spring-configuration-metadata.json
* add another test: similarly, if you have the properties alpha and beta in spring-configuration-metadata.json, we'll only consider alpha if dataflow-configuration-metadata.properties only has alpha
@anthonydahanne
Copy link
Member

@AamnaZahid I fixed a test, and added another one in a separate commit.

@AamnaZahid
Copy link
Author

AamnaZahid commented Oct 24, 2024 via email

@onobc
Copy link

onobc commented Oct 24, 2024

To elaborate a bit more on the link provided by @anthonydahanne .

Here is a little background on the Dataflow dedicated metadata. It is used to drive the UI/API during stream/task definition.

This describes why we have the dedicated metadata:
https://dataflow.spring.io/docs/applications/application-metadata/#dedicated-metadata-artifacts

This describes the metadata file:
https://dataflow.spring.io/docs/applications/application-metadata/#data-flow-configuration-metadata

This describes the metadata label:
https://dataflow.spring.io/docs/applications/application-metadata/#metadata-container-image-label

Here is a concrete example that uses 3 of the pre-built stream-applications (e.g. splitter used below that include said metadata.

The stream is defined as time | splitter | log and you can see in the UI the user is presented w/ only the properties that are in this resulting metadata (otherwise we would have to show them all N of the available Spring Boot config props).

Screenshot 2024-10-24 at 09 37 50 Screenshot 2024-10-24 at 09 37 58

I hope this helps w/ some more context.

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch A change requiring a patch version bump type:enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants