Skip to content

Fix check command #55

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.

Sign up for GitHub

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

Jump to bottom
Merged
vsoch merged 4 commits into from
Feb 28, 2022
Merged

Fix check command #55

vsoch merged 4 commits into from
Feb 28, 2022

Conversation

@PauloMigAlmeida
Copy link
Contributor

@PauloMigAlmeida PauloMigAlmeida commented Feb 28, 2022

fix singularity check command. If validation failed then an exception was being thrown instead of printing error message

Now it's working as expected:

ERROR /home/paulo/workspace/singularity-compose/singularity-compose-invalid.yml is not valid.

Process finished with exit code 1

Sorry about the previous PR, I had switched to another branch I'm working on and got confused...

@PauloMigAlmeida
fix singularity check command. If validation fails exception was bein…
4dfd9e1 
…g thrown instead of printing error message

Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
@PauloMigAlmeida
fix singularity check command. If validation fails exception was bein…
3301182 
…g thrown instead of printing error message

Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
@PauloMigAlmeida
Copy link
Contributor Author

PauloMigAlmeida commented Feb 28, 2022

@vsoch this is the right PR. What do you think?

@PauloMigAlmeida PauloMigAlmeida mentioned this pull request Feb 28, 2022
Closed
@PauloMigAlmeida
add changelog entry + bump version
3539247 
Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
vsoch
vsoch reviewed Feb 28, 2022
View reviewed changes
scompose/client/check.py
valid = validate_config(f)
if valid and not args.preview:
bot.info("%s is valid." % f)
elif result:
Copy link
Member

@vsoch vsoch Feb 28, 2022

Choose a reason for hiding this comment

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

Same question as before! What about the case when it's valid and a preview and the error says mistakenly "X is not valid?"

Copy link
Contributor Author

@PauloMigAlmeida PauloMigAlmeida Feb 28, 2022

Choose a reason for hiding this comment

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

Oops 😅 good catch. On further reflection, there is no reason why args.preview should be part of that if stmt, right?

Copy link
Contributor Author

@PauloMigAlmeida PauloMigAlmeida Feb 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

In my head, preview is only about whether we should print the deep-merged config file.

If the attempt of using it here was to make it behave like a "quiet mode" check (for scripting purposes) then the previous impl needs a different tweak as the exception thrown would not let that happen as it is, right?

Update: I found the rationale around that: #48 (comment)

Copy link
Contributor Author

@PauloMigAlmeida PauloMigAlmeida Feb 28, 2022

Choose a reason for hiding this comment

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

I updated the logic now. My brain stops working after 22ish..so if I missed anything, let me know :)

Copy link
Member

@vsoch vsoch Feb 28, 2022

Choose a reason for hiding this comment

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

haha no worries! I'm at 2am over here I really need to stop programming... 😆 ... can't... !

Copy link
Contributor Author

@PauloMigAlmeida PauloMigAlmeida Feb 28, 2022

Choose a reason for hiding this comment

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

Wow! I hope I'm not the one disturbing your sleeping pattern... this PR can wait for sure 😅

@PauloMigAlmeida
re-enable quiet mode for check command
6214265 
Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
@vsoch
Copy link
Member

vsoch commented Feb 28, 2022

I think this looks good! If tests are passing and you can confirm you are happy with the behavior locally, I can merge.

@PauloMigAlmeida
Copy link
Contributor Author

PauloMigAlmeida commented Feb 28, 2022

Last local test:

wrong && wrong + preview (which should have the same behavior):

singularity-compose/scompose/client/__init__.py -f .../singularity-compose-replicas-invalid.yml check (--preview)
ERROR /home/paulo/workspace/singularity-compose/singularity-compose-replicas-invalid.yml is not valid.

Process finished with exit code 1

right:

singularity-compose/scompose/client/__init__.py -f .../singularity-compose-replicas-right.yml check
/home/paulo/workspace/singularity-compose/singularity-compose-replicas-right.yml is valid.

Process finished with exit code 0

right + --preview:

singularity-compose/scompose/client/__init__.py -f .../singularity-compose-replicas-right.yml check --preview

version: '2.0'
instances:
  helloworldlatest:
    image: helloworldlatest.sif
    start:
      args: arg0 arg1 arg2
    deploy:
      replicas: 1

Process finished with exit code 0

Seems to be in order 👍🏼

@vsoch vsoch merged commit 9764372 into singularityhub:master Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vsoch vsoch vsoch approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@PauloMigAlmeida @vsoch