Added new CLI option to support better control over managed object names #464
Conversation
…t naming used in imports and exports
|
There is no v2 branch - a branch would need to be created from v2.1.0 to allow this to correctly target the correct codebase. |
|
Hi, can someone please provide me with an indication if this change is under consideration? If it is I will add tests for it or modify as needed. Please don't discount this due to the conflicts. They are present as there is no 2.x branch to target. Also, I will add this to support v3.x if needed. |
|
@pmckeown Nappy New Year! :) And THANK YOU for your PR! We are working through a queue of some large PRs from contributing parties and I am super excited to get to yours. Please bear with us as we catch up. I will leave more comments here with questions or feedback. We do not follow the practice of release branches to keep our code maintenance to a minimum. Your PRs should always go against main and will be merged into main and become immediately available in the next pre-release. |
Hi Volker, thanks for the response. I guess that means it needs to be compatible with v3 - though our codebase at the minute is not due to some breaking changes from v2 to v3. Once we get our codebase up to v3 and the deployments working using that version I will redo this PR as you are essentially saying that no additional fixes/changes to the previous releases will be made right? |
|
@pmckeown Yes, you are right. In rare instances we have created a feature branch (1.x) but that was due to the fact that 2.x was basically a re-write and absolutely incompatible but we had a user based sitting on 1.x and so we needed to be able to fix bugs for a while until everybody made the upgrade to 2.x. The 3.x release really is not that much different from 2.x but it contains breaking export/import format changes in a few places, so we decided to make a major version bump. If I may ask: What is breaking you in 3.x? |
|
There were a few little things that blocked us from upgrading. E.g. there was a bug in the JSON mapping of the journey file structure in the new codebase, or we needed to update our journey config files from |
|
Hi @vscheuber - do you think this will be merged soon? The corresponding change in frodo-lib is merged to the main branch now, so just need this to use that feature in the next release. |
phalestrivir
left a comment
phalestrivir
left a comment
There was a problem hiding this comment.
I was just talking with @hfranklin and he was saying we need to get tests written around this change before we merge it in. I will get those added in later this week, but I'm rejecting the PR for now until they are in
OK thats a good plan. Thank you! |
|
@phalestrivir did you get a chance to create some tests for this? I've had a look at the test framework in the past but looks hard to get your head around. Would appreciate your support on getting this merged, as we need this functionality but would like to take advantage of the latest features and patches in frodo too. |
@pmckeown Sorry, I haven't gotten the tests created yet because I realized I'll need a Forgeops deployment to test it against (since we need to create mock recordings for the tests, and since this is a feature specific to Forgeops). I haven't created a Forgeops deployment before, so I will need to create one for testing before I can get those created. I'll try to see if I can get that done this week, assuming it doesn't take too long for me to get one spun up. I'll make sure to update you once I have it finished. |
|
@pmckeown I'm posting another update here. I tried to get a Forgeops deployment working in a VM, but so far I haven't had any luck, the AM cluster doesn't seem to startup correctly, and I've tried everything I can think of. What I'm thinking of doing is, instead of trying to install Forgeops, I am going to create tests against an IDM deployment that I already have setup once we have our PR for IDM on prem connections finished and merged in. I'm just waiting on a review from @hfranklin on that, and then I'll put up a PR against this repo for it, and once it's all merged in I can create a PR with the tests. I already have a branch with the tests on it here, but I will have to modify them to work for an on prem IDM deployment since I haven't been able to get the Forgeops deployment working. |
Thanks for the update! If there is anything I can do to help with running the tests then I can do. Is there a guide on how to run the tests suites? Expectations around HAR file creation, use, storage etc? |
@pmckeown The documentation is contained in the test files themselves on how to record the HAR files using PollyJS. There are commands that you run that generate the HAR files. You can run the tests using We're still trying to get a ForgeOps instance up and running that we could use. I was originally trying to do it with Minikube which wasn't working for me, but I believe @hfranklin was able to get it working with a different cluster system. The goal I believe is to have a VM (or possibly Docker Image) which we can spin up for running ForgeOps whenever we need it for testing Frodo. Once we have that, I'll be able to update my branch with the tests, but if you already have one that you don't mind using for creating those test recordings feel free to use that one. |
|
Adding this here too, for keeping the conversation in one place: Is there a way we can move forward with this change without the need for the forgeOps deployment recording? I feel like the forgeops use case testing needs a bit more thought. The deployment for adding additional test cases may not be homogenous. In fact our deployment uses our Midships Ping AIS Accelerator, not ForgeOps, to provide the deployment. Just Frodo thinks its a ForgeOps deployment. But as I explain in the above link, the domain used to access our deployment is internal, and while I can create a HAR to provide the stubs, this will be defined with our internal address and might vary slightly from ForgeOps, like in the OAuth Client name using in OAuth flows. I have run the test suite, and regenerated the snap files so that they contain the new CLI argument. I don't know how to move forward with this PR. Any inputs would be appreciated. |
| host AM base URL, e.g.: https://cdk.iam.example.com/am. To use a connection profile, just specify a unique substring. | ||
| username Username to login with. Must be an admin user with appropriate rights to manage authentication journeys/trees. | ||
| password Password. |
There was a problem hiding this comment.
Try as I might, I could not get the snap files to be generated using an 80 character column width. Can someone share the command so I can regenerate these snap files with only the additional argument being changed?
There was a problem hiding this comment.
@phalestrivir any hints on how to make the snaps 80 character width to avoid all of these wrapping changes in the snap files?
There was a problem hiding this comment.
Also I have some questions on the mocks:
- When I record them, they are recorded into a
defaultdirectory (e.g.test/e2e/mocks/default_2470140894/recording.har), but the rest of them are placed in named directories, liketest/e2e/mocks/app_527074092/export_4211608755/0_Ni_D_1857086194/am_1076162899/recording.har- what places them in this directory? Is it manual? How are they then referenced at runtime? Or are they only served up from the frodo-lib project - see next question - Do the recordings need to go into frodo-lib as per the docs in the tests? I cannot see any mock recordings related to the app export tests.
3 participants
Related to frodo-lib#480 - adds the CLI option
--use-realm-prefix-on-managed-objectsto support enabling this feature in the Lib.