Skip to content
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

Remove unsupported fields for DB roles show page #15573

Merged
merged 5 commits into from
May 25, 2022

Conversation

linda9379
Copy link
Contributor

@linda9379 linda9379 commented May 24, 2022

Removed unsupported field of revocation_statements for DB roles for elasticsearch database

@linda9379 linda9379 added the ui label May 24, 2022
@hashicorp-cla
Copy link

hashicorp-cla commented May 24, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@hellobontempo hellobontempo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job! on your first PR! 🎉 🎉 🎉 A couple comments to address - let me know if you have any questions or want help refactoring!

I recommend updating the PR title to help explain what is improved/fixed by your PR (something like "Remove unsupported fields for DB roles show page") and add a description, however brief. All of that makes it easier to grasp the work tackled and for posterity when searching historical PRs in the future. Screenshots of before and after are also always welcome to help provide context as to what the code changed in the UI.

@@ -0,0 +1,3 @@
```release-note:bug
**UI**: Fixed unsupported revocation statements field for DB roles
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the standard format here is just ui: instead of **UI**. We typically reserve bold to call out feature work

@@ -3,7 +3,7 @@ import { computed } from '@ember/object';
import { alias } from '@ember/object/computed';
import lazyCapabilities, { apiPath } from 'vault/macros/lazy-capabilities';
import { expandAttributeMeta } from 'vault/utils/field-to-attrs';
import { getRoleFields } from '../../utils/database-helpers';
import { getRoleFields, getStatementFields } from '../../utils/database-helpers';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I originally wrote this import! But it can actually be cleaned up and changed to vault/utils/database-helpers (which makes it easier to distinguish whether it's an addon util or a util in the main app)

@@ -91,7 +91,11 @@ export default Model.extend({

get showFields() {
let fields = ['name', 'database', 'type'];
fields = fields.concat(getRoleFields(this.type)).concat(['creation_statements', 'revocation_statements']);
fields = fields.concat(getRoleFields(this.type)).concat(['creation_statements']);
const plugin = this.database + '-database-plugin';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like oracle doesn't use the convention of <database>-database-plugin so it'd fail the conditional below even though it does have a revocation_statements field. Also, it's slightly misleading, but MongoDB has revocation_statement singular (because it has slightly different formatting when we connect the DB in the UI) but we still want the plural field in showFields because that's how the parameter is returned from the backend.

Since - at this time - elasticsearch appears to be the only database that doesn't have the revocation_statements attr perhaps we can refactor the conditional and concat that field for all databases except elasticsearch

@linda9379 linda9379 changed the title unsupported fields display for db roles Remove unsupported fields for DB roles show page May 24, 2022
@linda9379 linda9379 requested a review from hellobontempo May 24, 2022 18:43
Copy link
Contributor

@hellobontempo hellobontempo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nearly there! Just one more small change. This PR also needs a milestone - 1.11.0-rc1 is probably good here!

@@ -91,7 +91,10 @@ export default Model.extend({

get showFields() {
let fields = ['name', 'database', 'type'];
fields = fields.concat(getRoleFields(this.type)).concat(['creation_statements', 'revocation_statements']);
fields = fields.concat(getRoleFields(this.type)).concat(['creation_statements']);
if (this.database != 'elasticsearch') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this works, it's best practice to use a strict equals (=== or !==). I also think this would be a great place for a comment so when someone returns to this part of the code, they quickly understand why elasticsearch was excluded.

Suggested change
if (this.database != 'elasticsearch') {
// elasticsearch does not support revocation statements: https://www.vaultproject.io/api-docs/secret/databases/elasticdb#parameters-1
if (this.database[0] !== 'elasticsearch') {

@linda9379 linda9379 added this to the 1.11.0-rc1 milestone May 24, 2022
@linda9379 linda9379 requested a review from hellobontempo May 24, 2022 20:22
Copy link
Contributor

@hellobontempo hellobontempo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 well done!!

@linda9379 linda9379 merged commit bf1c082 into main May 25, 2022
@linda9379 linda9379 deleted the ui/VAULT-3803/unsupported_fields_display_for_DB_roles branch May 25, 2022 15:30
Gabrielopesantos pushed a commit to Gabrielopesantos/vault that referenced this pull request Jun 6, 2022
* Fixed unsupported revocation statements field display for DB roles

* Fixed linting

* Added changelog

* Fixed conditional to filter for only elasticsearch database and changed format of text in changelog

* Fixed conditional and added comment for bug fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants