Skip to content

add helio auth to TriggerAnalysisCallback and centre_sample_plan - #197

Merged
jacob-williamson merged 4 commits into
mainfrom
add_helio_auth
Nov 9, 2026
Merged

jacob-williamson merged 4 commits into
mainfrom
add_helio_auth

Conversation

@RJCD-Diamond

@RJCD-Diamond RJCD-Diamond commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Heliotrapi will need to be updated in the i15-services repo to 0.4.1 when this is deployed

@codecov

codecov Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.90%. Comparing base (edc9c9d) to head (32864fd).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #197      +/-   ##
==========================================
+ Coverage   91.89%   91.90%   +0.01%     
==========================================
  Files          16       16              
  Lines         481      482       +1     
==========================================
+ Hits          442      443       +1     
  Misses         39       39              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@DominicOram DominicOram left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, just the one major comment.

Could: I think we should have a couple of unit tests on this, having to type it out twice may have caught https://github.com/DiamondLightSource/crystallography-bluesky/pull/197/changes#r4378644201

Comment on lines +18 to +19
username: str | None = None,
password: str | None = None,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could: What is the usecase for not providing a username/password? Are there some endpoints that don't need it? Or some test tiled instances that don't need it?

@RJCD-Diamond RJCD-Diamond Oct 26, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Heliotrapi can be confgured to not have auth, so username and password are set to None, it's str | None to match that. There are also some endpoints which don't

However for i15-1, it should always have a password so we could always check if None and raise?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sure, I guess if we get round to running system tests for this we would probably set something up w/o using auth so this might be useful. I'm guessing if we pass in None we'll get a 400 error anyway so should be relatively obvious what's gone wrong without having to explicitly check here. Let's leave it

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, it will return an unauthorised error

Co-authored-by: Dominic Oram <dominic.oram@diamond.ac.uk>

@DominicOram DominicOram left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, tests would be good but not that bothered.

@jacob-williamson
jacob-williamson merged commit e710f75 into main Nov 9, 2026
9 checks passed
@jacob-williamson
jacob-williamson deleted the add_helio_auth branch November 9, 2026 10:34
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