-
Notifications
You must be signed in to change notification settings - Fork 113
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
Added Ids column in Policies and Roles List #4393
Conversation
Deploy preview for chef-automate processing. Building with commit 62f53f9 https://app.netlify.com/sites/chef-automate/deploys/6012862a5721f10007db7dc6 |
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.
LGTM
|
15c2cac
to
9e32a4c
Compare
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.
Sorry this is the wrong way to fix the tests, I'm afraid. Can you alter the assertions to match what's in the policy list, and add one for the policy IDs to be displayed, please?
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.
Also, as I mentioned several days ago, it is important not to squash your changes into a single commit, especially when making changes responding to reviewer feedback.
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.
Added a note on how to adapt the tests! :)
components/automate-ui/src/app/modules/policy/list/policy-list.component.html
Show resolved
Hide resolved
9e32a4c
to
53ce007
Compare
This PR still needs to add tests for the ID column as srenatus indicated above. |
53ce007
to
0b37bca
Compare
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.
Looks like the requested test changes have been done, so I am approving, but since this is so old, though, I think it would be prudent to rebase before merging to see if all the tests are still good.
Signed-off-by: samshinde <ashinde@chef.io>
Signed-off-by: samshinde <ashinde@chef.io>
Signed-off-by: samshinde <ashinde@chef.io>
0b37bca
to
62f53f9
Compare
Signed-off-by: samshinde ashinde@chef.io
🔩 Description: What code changed, and why?
In the Roles and policy list tables added a column for ID.
⛓️ Related Resources
#4036
👍 Definition of Done
Added a column for ID on the policy list and role list pages.
👟 How to Build and Test the Change