Skip to content

Conversation

@jakeglobus
Copy link
Contributor

No description provided.

@jakeglobus jakeglobus force-pushed the add-index-roles-crud branch from bab9423 to f5e8c7d Compare February 6, 2025 17:13
@codecov
Copy link

codecov bot commented Feb 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.72%. Comparing base (8a9de93) to head (0a2637d).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #425      +/-   ##
==========================================
+ Coverage   98.71%   98.72%   +0.01%     
==========================================
  Files          71       72       +1     
  Lines        1242     1254      +12     
  Branches      210      210              
==========================================
+ Hits         1226     1238      +12     
  Misses         15       15              
  Partials        1        1              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@jbottigliero jbottigliero left a comment

Choose a reason for hiding this comment

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

Looks good, but just needs to be moved to a separate roles file and method names updated.

*
* @see https://docs.globus.org/api/search/reference/role_list/
*/
export const getRoles = function (
Copy link
Member

Choose a reason for hiding this comment

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

To match the other services and resources, I think this should actually be search.index.roles(.getAll | .create | .remove)

https://globus.github.io/globus-sdk-javascript/modules/_globus_sdk.html#service-method-names

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah oops, I'll get that fixed up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I think it's okay now. I'm not totally sure about the module export from search/index.ts though so please double check me there

Copy link
Member

Choose a reason for hiding this comment

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

👍 - I just swapped the changes in search/index.js for: https://github.com/globus/globus-sdk-javascript/pull/425/files#diff-9b2324bba31437d1ca01432b814e10bcf00777d71d1ffc0a23d3b2481bda0132R10

Should have the same effect, but keep things structured.

@jakeglobus jakeglobus force-pushed the add-index-roles-crud branch from f5e8c7d to cb173c9 Compare February 6, 2025 19:01
@jakeglobus jakeglobus force-pushed the add-index-roles-crud branch from cb173c9 to 913abae Compare February 6, 2025 19:05
Comment on lines +10 to 12
export * as roles from './roles.js';

/**
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah of course, thanks Joe!

@jbottigliero jbottigliero merged commit 1b40d08 into main Feb 6, 2025
9 checks passed
@jbottigliero jbottigliero deleted the add-index-roles-crud branch February 6, 2025 21:06
jbottigliero pushed a commit that referenced this pull request Feb 6, 2025
🤖 I have created a release *beep* *boop*
---


##
[5.5.0](v5.4.1...v5.5.0)
(2025-02-06)


### Features

* **Search:** Adds support for Search Index Roles CRUD methods.
([#425](#425))
([1b40d08](1b40d08))


### Bug Fixes

* **Transfer, Typescript:** Adds TransferErrorDocument type and improves
DirectoryListingError types.
([#420](#420))
([aadfbdf](aadfbdf))
* **Typescript, Search:** Improve the GFacet Search type
([#426](#426))
([1b245a5](1b245a5))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants