Skip to content

Initial#1

Merged
rerpha merged 14 commits intomainfrom
initial
Sep 3, 2025
Merged

Initial#1
rerpha merged 14 commits intomainfrom
initial

Conversation

@Tom-Willemsen
Copy link
Copy Markdown
Member

@Tom-Willemsen Tom-Willemsen commented Aug 24, 2025

See also ISISComputingGroup/ibex_utils#277 if you don't have rust locally

@Tom-Willemsen Tom-Willemsen force-pushed the initial branch 21 times, most recently from bc7629a to ec42d56 Compare August 24, 2025 11:28
@Tom-Willemsen Tom-Willemsen requested a review from rerpha September 1, 2025 16:17
Copy link
Copy Markdown
Contributor

@rerpha rerpha 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 and works well - just some minor comments

Comment thread src/lib.rs
Comment thread src/lib.rs
runs-on: ${{ matrix.os }}
strategy:
matrix:
os: [ "ubuntu-latest", "windows-latest" ]
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.

is there any point running the tests on linux if we wrap wincred?

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.

and is there benefit to running these as separate jobs rather than steps within a job?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ick json is usable on linux, although whether that's a useful enough thing by itself I'm unsure? Happy to just go to windows-only depending what you prefer?

(originally I also had an ick ssh, but that's superceded by just using key-based auth)...

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.

up to you really - maybe not needed

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'll leave as-is for the moment and we can revisit if we want / next time (if ever) we end up touching this tool.

Comment thread src/lib.rs
Comment thread src/credentials.rs Outdated
@rerpha rerpha merged commit 66157e9 into main Sep 3, 2025
5 checks passed
@Tom-Willemsen Tom-Willemsen deleted the initial branch October 15, 2025 08:24
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.

2 participants