Y24-220 Create v2 end point for tagsets #4732

seenanair · 2025-03-12T16:21:14Z

Changes proposed in this pull request

API v2 end point to query tag sets
Fetaure flag support (y24_220_enable_tag_set_api)

Instructions for Reviewers

[All PRs] - Confirm PR template filled
[Feature Branches] - Review code
[Production Merges to main]
- Check story numbers included
- Check for debug code
- Check version

harrietc52

Looks good!

Just the comment about adding the example requests for the Yard docs.

Whilst adding comments/ example requests for the other API v2 requests, I created a PostMan directory to persist mainly the POST requests, but also the GETs. If you would like to (or just to have a play) you can add these GET requests to the PostMan. Ref here

app/resources/api/v2/tag_set_resource.rb

harrietc52 · 2025-03-19T11:13:23Z

app/resources/api/v2/tag_set_resource.rb

+    #
+    # This resource represents a TagSet, which links together two related tag groups.
+    # It includes the following attributes and relationships:
+    #


Would it be possible to include the example GET requests, including the filter examples?

I’ve added example requests as part of updated docs here. You can then use the GitHub action to rebuild the Yard Syntax and view these comments/ examples here

@harrietc52 , I've updated the documentation as well as the Sequencescape API v2.postman_collection.json in smb://files-smb/pipeline_solutions. Please check if it is ok.

I think you might need to run this Workflow, as described here, to update the SS Yard documentation on GitHub pages :)

Postman looks good, thank you

spec/requests/api/v2/tag_sets_spec.rb

…d show actions

KatyTaylor

Looks mostly good, thank you! Just a couple of questions to look at.

KatyTaylor · 2025-03-21T14:28:23Z

app/api/endpoints/tag_sets.rb

+  model {}
+
+  instance {}
+end


Was this necessary for some reason? We shouldn't be adding new V1 endpoints - Stuart's project was to replace remaining V1 use with V2 API.

I was not sure about this and I just copied how it was for other end points. :D It sounds like it is for supporting V1. Shall I remove this?

Yes pretty sure this is just for V1 so unnecessary.

KatyTaylor · 2025-03-21T14:30:05Z

app/controllers/api/v2/tag_sets_controller.rb

+
+      def check_feature_flag
+        render json: { error: 'TagSets API is disabled' } unless Flipper.enabled?(:y24_220_enable_tag_set_api)
+      end


Think there should there be an addition to feature_flags.yml in this PR?

Also, if you haven't already, would you mind creating a story and a pull request to remove the feature flag, so it's easy to remove when we decide to? There's a team feature flags guidance page on Confluence in case if you haven't seen it.

Done 👍 #4766

Thank you 🙏

I think the idea was to go even further, and create the (draft) pull request ready to merge in whenever appropriate - so that removing the feature flag is really effortless (obviously extra effort for you upfront, but hopefully less overall since it's fresh in your head).

Also, thinking about when to remove the flag - on the story you've written "once the prerequisite work (Y25-185) is completed".

We can definitely switch the new feature 'on' once Y25-185 is completed, but I think we could let the flag hang around for a couple of months or so afterwards, just to ensure the page gets used and we haven't missed anything?

This was what I thought about the level of risk (copied from Slack):

I think there is a small remaining risk, as we are making the functionality on that page more constrained, in that we're not including ALL the tag groups, and we're not allowing you to select any possible combination of tag 1 and tag 2.
So feature flagging could help in that we could resolve any prod issues immediately by flipping the flag.

Since you've implemented the flag, I feel we may as well keep it around for a little while in case we get any production issues due to the above?

Oh, okay. I agree, and all of your comments make complete sense. I will create a draft PR and chang the timing to remove the flag.

@KatyTaylor
Following draft PRs created which enable the removal of
SS - #4770
Limber - sanger/limber#2275
Integration suite - https://gitlab.internal.sanger.ac.uk/psd/integration-suite/-/merge_requests/232

KatyTaylor

Looks good, thank you, although a couple of tests are still failing

seenanair · 2025-03-24T16:18:24Z

@harrietc52 @KatyTaylor I've addressed all your comments, and now all tests are passing. Please review and let me know if any further changes are needed.

harrietc52 · 2025-03-26T10:43:47Z

@harrietc52 @KatyTaylor I've addressed all your comments, and now all tests are passing. Please review and let me know if any further changes are needed.

Looks great! Just the command to run to update the SS GitHub Pages for Yard documentation as mentioned above 😄

seenanair · 2025-03-26T10:51:51Z

@harrietc52 @KatyTaylor I've addressed all your comments, and now all tests are passing. Please review and let me know if any further changes are needed.

Looks great! Just the command to run to update the SS GitHub Pages for Yard documentation as mentioned above 😄

@harrietc52 Thanks for the review! 🙏 I ran the action 14081314706 against branch y24_220_create_v2_end_point_for_tagsets , but the documentation hasn’t been updated with the Tagset API but worked fine locally. Did I miss something?

harrietc52 · 2025-03-27T09:38:29Z

@harrietc52 Thanks for the review! 🙏 I ran the action 14081314706 against branch y24_220_create_v2_end_point_for_tagsets , but the documentation hasn’t been updated with the Tagset API but worked fine locally. Did I miss something?

Oh strange! That is odd. I’ll try give it a go too

seenanair added 5 commits March 12, 2025 14:59

Add TagSets endpoint and resource for API V2

9d681ed

Add TagSetsController for API V2

6ddec84

Updates

4f89622

Add tests for TagSets API and TagSet resource in API V2

1901b4c

rubocop updates

f851c3b

seenanair requested review from KatyTaylor and yoldas March 13, 2025 09:19

Add feature flag check for TagSets API in controller

228877a

seenanair requested a review from andrewsparkes March 18, 2025 10:34

harrietc52 approved these changes Mar 19, 2025

View reviewed changes

seenanair added 2 commits March 21, 2025 13:04

Merge branch 'develop' into y24_220_create_v2_end_point_for_tagsets

71fec17

Enhance TagSet resource documentation and restrict routes to index an…

5430b8f

…d show actions

KatyTaylor requested changes Mar 21, 2025

View reviewed changes

seenanair added 2 commits March 21, 2025 15:07

Add feature flag for Tag Set API readiness

d7f87f6

Delete tag_sets.rb

cefff16

KatyTaylor approved these changes Mar 24, 2025

View reviewed changes

seenanair added 2 commits March 24, 2025 10:17

Merge branch 'develop' into y24_220_create_v2_end_point_for_tagsets

f6d83b7

test: update

c808a55

seenanair requested review from KatyTaylor and harrietc52 March 24, 2025 16:18

harrietc52 approved these changes Mar 26, 2025

View reviewed changes

Merge branch 'develop' into y24_220_create_v2_end_point_for_tagsets

c8b2078

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Y24-220 Create v2 end point for tagsets #4732

Y24-220 Create v2 end point for tagsets #4732

seenanair commented Mar 12, 2025 •

edited

Loading

harrietc52 left a comment

harrietc52 Mar 19, 2025 •

edited

Loading

seenanair Mar 21, 2025

harrietc52 Mar 26, 2025

harrietc52 Mar 26, 2025

KatyTaylor left a comment

KatyTaylor Mar 21, 2025

seenanair Mar 21, 2025

KatyTaylor Mar 24, 2025

KatyTaylor Mar 21, 2025

KatyTaylor Mar 21, 2025

seenanair Mar 21, 2025

KatyTaylor Mar 24, 2025

seenanair Mar 24, 2025

seenanair Mar 24, 2025

KatyTaylor left a comment

seenanair commented Mar 24, 2025 •

edited

Loading

harrietc52 commented Mar 26, 2025

seenanair commented Mar 26, 2025 •

edited

Loading

harrietc52 commented Mar 27, 2025 •

edited

Loading

Y24-220 Create v2 end point for tagsets #4732

Are you sure you want to change the base?

Y24-220 Create v2 end point for tagsets #4732

Conversation

seenanair commented Mar 12, 2025 • edited Loading

Changes proposed in this pull request

Instructions for Reviewers

harrietc52 left a comment

Choose a reason for hiding this comment

harrietc52 Mar 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KatyTaylor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KatyTaylor left a comment

Choose a reason for hiding this comment

seenanair commented Mar 24, 2025 • edited Loading

harrietc52 commented Mar 26, 2025

seenanair commented Mar 26, 2025 • edited Loading

harrietc52 commented Mar 27, 2025 • edited Loading

seenanair commented Mar 12, 2025 •

edited

Loading

harrietc52 Mar 19, 2025 •

edited

Loading

seenanair commented Mar 24, 2025 •

edited

Loading

seenanair commented Mar 26, 2025 •

edited

Loading

harrietc52 commented Mar 27, 2025 •

edited

Loading