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

[Fix] jupyter notebook spawn checks #1228

Merged
merged 1 commit into from
Feb 26, 2024

Conversation

jstourac
Copy link
Member

@jstourac jstourac commented Feb 26, 2024

This is a fix for the recent change [1,2]


CI:

Screenshot from 2024-02-26 10-59-28

@jstourac jstourac self-assigned this Feb 26, 2024
Copy link
Contributor

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass %
424 0 0 424 100

[Arguments] ${image_name} ${version}
${ide}= Notebook Expected Ide ${image_name}
[Arguments] ${image} ${version}
${ide}= Notebook Expected Ide ${image}

Check warning

Code scanning / Robocop

The assignment sign is not consistent within the file. Expected '{{ expected_sign }}' but got '{{ actual_sign }}' instead Warning test

The assignment sign is not consistent within the file. Expected ' =' but got '=' instead
This is a fix for the recent change [1,2]. Let's be consistent in the
variable naming in all relevant keywords.

* [1] red-hat-data-services#1102
* [2] 60fffb5
@jstourac jstourac changed the title [Fix] post-install jupyter notebook spawn checks [Fix] jupyter notebook spawn checks Feb 26, 2024
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@jiridanek jiridanek added verified This PR has been tested with Jenkins enhancements Bugfixes, enhancements, refactoring, ... in tests or libraries (PR will be listed in release-notes) labels Feb 26, 2024
Copy link
Member

@jiridanek jiridanek left a comment

Choose a reason for hiding this comment

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

I'd expect variable image to hold something like "quay.io/modh/notebooks/...@sha256-...", so maybe image_name is a slightly better name. But up to you, I guess there are reasons I'm not seeing.

[Arguments] ${image_name} ${version}
${ide}= Notebook Expected Ide ${image_name}
[Arguments] ${image} ${version}
${ide}= Notebook Expected Ide ${image}
Copy link
Contributor

Choose a reason for hiding this comment

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

you're just changing variable name, what is this PR fixing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Few lines below, there was just image used in the method, which didn't work with the current state. My first intention was to rename that variable to image_name also, but then I realised that everywhere else we use just image...

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, it's hidden bcos unchanged. Ok thanks

@jstourac
Copy link
Member Author

I'd expect variable image to hold something like "quay.io/modh/notebooks/...@sha256-...", so maybe image_name is a slightly better name. But up to you, I guess there are reasons I'm not seeing.

I agree, but everywhere else there is used just image, thus I'm making this cosistent.

@jstourac jstourac requested a review from bdattoma February 26, 2024 10:16
@jstourac jstourac merged commit 410104c into red-hat-data-services:master Feb 26, 2024
11 checks passed
@jstourac jstourac deleted the fixJupyterSpawn branch February 26, 2024 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancements Bugfixes, enhancements, refactoring, ... in tests or libraries (PR will be listed in release-notes) verified This PR has been tested with Jenkins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants