AssetId support for adapters#8
Conversation
- updated readme - updated to use LdapNameGeneratorUtil - added and made tests for LdapNameGeneratorUtil - added adapter dependent resource test - added tests for adapter - added assets to fint adapter - updated ghactions - added dependabot
sivertheisholt
left a comment
sivertheisholt
left a comment
There was a problem hiding this comment.
Good work, I dont know this operator that much, so left a few comments regarding assets/assetIds that im confused on. Should maybe also get @murillio4 to look over.
There was a problem hiding this comment.
Missing:
Azure/k8s-artifact-substitute@v2
Without the azure/k8s-deploy@v6 wont work
There was a problem hiding this comment.
Wrong. Finterator does not use FlaisApplication which does not work with newer versions of azure/k8s-deploy. azure/k8s-deploy@v6 will work with normal k8s Deployments, which finterator does use.
| private String orgId; | ||
| private String note; | ||
| private List<String> components = Collections.emptyList(); | ||
| private List<String> assets = Collections.emptyList(); |
There was a problem hiding this comment.
I see the use of assets and assetIds. What is the diff?
If they are the same, it seems inconsistent?
| .build(); | ||
| primary.getSpec().getComponents() | ||
| .forEach(component -> adapter.addComponent(String.format("ou=%s,ou=components,o=fint", component))); | ||
| primary.getSpec().getAssets() |
There was a problem hiding this comment.
New created adapter gets values in assetIds, but existing adapter in assets?
There was a problem hiding this comment.
My bad on that, just pure inconsistency. Mattis and I had a conversation about the naming, and i messed that up in the confusion around it. Will make a decision to use either asset or assetIds. assetIds would make it more verbose, but asset looks cleaner in the setup.
There was a problem hiding this comment.
I think we should use assets and not assetIds
There was a problem hiding this comment.
Wrong. Finterator does not use FlaisApplication which does not work with newer versions of azure/k8s-deploy. azure/k8s-deploy@v6 will work with normal k8s Deployments, which finterator does use.
There was a problem hiding this comment.
You should add dependabot config for Docker and Gradle/Maven as well
There was a problem hiding this comment.
I think using a naming convention similar to whats purposed in kafkarator PR 7 is better.
I think the method is more appropriate because its simpler, and reproducible. Especially since it uses a hash instead of using RandomStringUtils.randomAlphabetic.
| .build(); | ||
| primary.getSpec().getComponents() | ||
| .forEach(component -> adapter.addComponent(String.format("ou=%s,ou=components,o=fint", component))); | ||
| primary.getSpec().getAssets() |
There was a problem hiding this comment.
I think we should use assets and not assetIds
3 participants
Added support for assetId in adapters (FLA-191). Had to rework (FLA-839) as there became a lot of merge conflicts after stalling previous PR.
Made changes to GH Actions so it's easier to deploy and test in test environment after PR is completed.
Work done: