-
Notifications
You must be signed in to change notification settings - Fork 77
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
Feature/add pim resources #727
Feature/add pim resources #727
Conversation
/test-examples="examples/authorization/pimactiveroleassignment.yaml" |
/test-examples="examples/authorization/pimeligibleroleassignment.yaml" |
358d754
to
1f199dc
Compare
Sorry for the mulitple ammends, had forgotton to sign off the commits. Should be in place now. |
/test-examples="examples/authorization/pimactiveroleassignment.yaml" |
/test-examples="examples/authorization/pimeligibleroleassignment.yaml" |
The check-diff is failing for the resources that were changed automatically by just running make reviewable. All changes are changing “ to ``, and they are probably changed back when the script is run. Could this be an issue with locale/charset? I see other pull requests have the same issue as well. |
Hi @knutejoh, thank you for your effort in this PR. Could you please run |
@turkenf `
Generated 723 resources!
nothing to commit, working tree clean` |
As you mentioned, the issue may be caused by your locale. When I run
|
What is your locale set to when you run the command? I see pull request #725 also has the same issue with the same resources. |
I encountered this issue the first time and my teammate @sergenyalcin said that he had encountered the same issue before. It works fine in my local with the following settings:
Please try the above settings and if the issue continues to reoccur, uncommit the changes to the following files:
|
1f199dc
to
6d36e43
Compare
I removed the changes in the mentioned files as requested :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution @knutejoh, I left a small comment and explanation about testing below.
Additionally, if newly added resources cannot be tested automatically, we prefer to first test them manually and write the results in the description. The two resources added here have some specific parameters (principalId
, roleDefinitionId
, scope
), so it does not seem possible to test them with uptest at the beginning.
Here we have rough testing steps when adding a new resource. Please put the logs/screenshots in the How has this code been tested
section after testing it manually.
Finally got it tested, added some comments in the initial comment. |
/test-examples="examples/authorization/roledefinition.yaml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your tests and comments.
I have defined values for scope
, principalId
and roleDefinitionId
to be used in automatic tests for the data that I recommend to you. I tested it locally and it worked, but I'm not sure if it will work in our test account. If it does not work, we can add a manual intervention and merge this PR.
I also left a comment for you to consider regarding the startDateTime
field.
Hey @knutejoh, We appreciate your efforts so far to include these resources. If you'd like to proceed with merging this PR into an upcoming release, please review the comments above. If there's anything you don't understand, feel free to ask for clarification. |
I've implemented the suggested changes, ended up removing the startDateTime field as it's optional. |
/test-examples="examples/authorization/pimactiveroleassignment.yaml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for addressing the last review comments, the ID we defined did not work for the test account, so for now we can add manual intervention to avoid blocking this PR and get these resources for the next release. And can you please solve the conflicts?
Hi @knutejoh, Thank you for being so patient here. Things seem a bit confusing here after this change. I see that you have rebased the recent changes and added First of all, since we have just added these resources, we want their versions to be Let me simply explain what we need to do below:
With the last changes, we add the above to override the configurations we made here and generate the
|
Signed-off-by: Knut-Erik Johnsen <abstract@knut-erik.org>
c0fc2bc
to
245ac1b
Compare
Hi. I've updated the code and reran the tests using kind, and I'm able to create both types of resources. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We really appreciate the effort you put into this PR @knutejoh, I left two small comments, we can merge once you add the license statements.
Signed-off-by: Knut-Erik Johnsen <abstract@knut-erik.org>
Description of your changes
Fixes #710
Adding azurerm_pim_eligible_role_assignment and azurerm_pim_active_role_assignment objects to the provider.
I have:
make reviewable
to ensure this PR is ready for review.backport release-x.y
labels to auto-backport this PR if necessary.How has this code been tested
Setup kind with the new resources
make run
Added resources with manually entered principalId, scope and roledefinition, using the new definition without lists.
Crossplane successfully created the resources in Azure