-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Add EE to the settings page #9653
Add EE to the settings page #9653
Conversation
return data; | ||
}, [executionEnvironmentId]) | ||
); | ||
|
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.
Since the settings API does not have summary_fields
. It was necessary an extra request for Details/Edit screens in order to fetch the EE and display the proper name.
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, I did not add a Revert
for the EE lookup. Since the lookup is different from the other components rendered on that screen.
running e2e tests. |
Build succeeded.
|
a5051de
to
530e158
Compare
Build succeeded.
|
Hey @nixocio some feedback, check if this make sense:
|
d4841bb
to
e6ea567
Compare
@tiagodread, please, take another look. Details page. Edit page. |
Build succeeded.
|
case 'choice': | ||
case 'field': | ||
case 'string': |
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.
Great idea!
"type": "field", | ||
"required": false, | ||
"label": "Global default execution environment", | ||
"help_text": ".", |
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.
"help_text" should match the changes added in /main/conf.py
@@ -76,6 +77,32 @@ const SettingGroup = withI18n()( | |||
) | |||
); | |||
|
|||
const ExecutionEnvironmentLookupField = withI18n()(({ i18n }) => { |
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.
Since this lookup is used in only one place (MiscSystemEdit) within Settings, it probably doesn't need to be in SharedFields and instead we could import the lookup directly from /components in MiscSystemEdit.
@marshmalien, updated as per your comments. |
Build succeeded.
|
}); | ||
await waitForElement(newWrapper, 'ContentLoading', el => el.length === 0); | ||
|
||
assertDetail(newWrapper, 'Access Token Expiration', '1 seconds'); |
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 could probably remove some of these detail label and value assertions since we check them in the previous test.
Build succeeded.
|
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! Tests are great and I like how you cleaned up the shared setting details.
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
Allow a system admin to set the global default execution environment. See: ansible#9088 This PR is also addressing the issue: ansible#9669
Build succeeded.
|
Build succeeded (gate pipeline).
|
Allow a system admin to set the global default execution environment.
See: #9088
This PR is also addressing the issue: #9669
Edit:
Details