Skip to content
This repository was archived by the owner on Jan 19, 2018. It is now read-only.

Wrong provider name in answers.conf, exits AtomicApp with readable error #563

Merged
cdrage merged 1 commit into from
Mar 4, 2016
Merged

Wrong provider name in answers.conf, exits AtomicApp with readable error #563

cdrage merged 1 commit into from
Mar 4, 2016

Conversation

@surajssd
Copy link
Collaborator

@surajssd surajssd commented Feb 17, 2016

When provider name in answers.conf is wrong then AtomicApp fails with stack-trace and no error output which is user understandable. This happened because there was no check for the condition if
given provider_key does not match with any of the providers that are supported for now.

Fixes Issue #562

dustymabe
dustymabe reviewed Feb 17, 2016
View reviewed changes
atomicapp/nulecule/lib.py Outdated

import logging

from atomicapp import set_logging
Copy link
Contributor

@dustymabe dustymabe Feb 17, 2016

Choose a reason for hiding this comment

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

where is this used?

Copy link
Contributor

@rtnpro rtnpro Feb 17, 2016

Choose a reason for hiding this comment

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

@surajssd remove unused import :)

@dustymabe
Copy link
Contributor

dustymabe commented Feb 17, 2016

@rtnpro @cdrage can you review this? Some questions to answer:

Should this issue be fixed in this way? Is there a better way? Should we sys.exit() straight from the code as is done here?

@dustymabe
Copy link
Contributor

dustymabe commented Feb 17, 2016

@rtnpro should we also write a test for this?

rtnpro
rtnpro reviewed Feb 17, 2016
View reviewed changes
atomicapp/nulecule/lib.py Outdated
@@ -1,11 +1,18 @@
# -*- coding: utf-8 -*-
import sys

Copy link
Contributor

@rtnpro rtnpro Feb 17, 2016

Choose a reason for hiding this comment

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

Unnecessary new line

@rtnpro
Copy link
Contributor

rtnpro commented Feb 17, 2016

@surajssd could you have a look at https://travis-ci.org/projectatomic/atomicapp/builds/109825603? I think with the suggested refactor, it should be OK.

@surajssd surajssd force-pushed the sane-error-on-wrong-provider branch from 0cd8e76 to eae57d1 Compare February 18, 2016 06:54
@rtnpro
Copy link
Contributor

rtnpro commented Feb 18, 2016

@surajssd Mind adding unittests for getProvider?

dustymabe
dustymabe reviewed Feb 18, 2016
View reviewed changes
atomicapp/plugin.py Outdated
raise NuleculeException("Invalid Provider - '{}', provided in "
"answers.conf (choose from 'docker', "
"'kubernetes', 'openshift', 'marathon')"
.format(provider_key))
Copy link
Contributor

@dustymabe dustymabe Feb 18, 2016

Choose a reason for hiding this comment

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

If we choose to tell them what options they can use then we should be using the PROVIDERS constant from constants.py and not a hardcoded list.

Copy link
Collaborator Author

@surajssd surajssd Feb 22, 2016

Choose a reason for hiding this comment

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

yes can do that!

@surajssd surajssd force-pushed the sane-error-on-wrong-provider branch 4 times, most recently from 0813d76 to 87ed1ad Compare February 22, 2016 07:18
rtnpro
rtnpro reviewed Feb 22, 2016
View reviewed changes
atomicapp/plugin.py Outdated
return provider

# importing this on top of file leads to circular import
from atomicapp.nulecule.exceptions import NuleculeException
Copy link
Contributor

@rtnpro rtnpro Feb 22, 2016

Choose a reason for hiding this comment

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

local imports are kind of hacks, and I prefer to avoid them. Let's not reference Nulecule* things in plugin.py at all, because, plugin.py is a dependency for Nulecule* and other things. Does it make sense?

Copy link
Collaborator Author

@surajssd surajssd Feb 22, 2016

Choose a reason for hiding this comment

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

Yes makes sense, but then what exception do I raise here?

Copy link
Contributor

@rtnpro rtnpro Feb 22, 2016

Choose a reason for hiding this comment

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

So, as I had said earlier, let's raise an exception in nulecule/main.py when provider is None.

Copy link
Contributor

@rtnpro rtnpro Feb 22, 2016

Choose a reason for hiding this comment

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

No need to raise any exception in plugin.py. It returns None because it's not able to find a provider. Simple, isn't it?

Copy link
Collaborator Author

@surajssd surajssd Feb 22, 2016

Choose a reason for hiding this comment

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

yes makes sense!! :)

@surajssd surajssd force-pushed the sane-error-on-wrong-provider branch from 87ed1ad to aa9414f Compare February 22, 2016 08:51
@rtnpro
Copy link
Contributor

rtnpro commented Feb 22, 2016

@surajssd One last thing, could you add a unittest for get_provider(), https://github.com/projectatomic/atomicapp/pull/563/files#diff-093821a6e6f49d13d49fdcb4f911695dR91, in tests/units/nulecule/test_lib.py?

@surajssd surajssd force-pushed the sane-error-on-wrong-provider branch 2 times, most recently from f8cbf5d to ea12f35 Compare February 23, 2016 09:02
@rtnpro
Copy link
Contributor

rtnpro commented Feb 23, 2016

LGTM 👍 @dustymabe shall we merge it in to master?

@dustymabe
Copy link
Contributor

dustymabe commented Feb 23, 2016

@rtnpro I trust you. :) - merge away.

@dustymabe
Copy link
Contributor

dustymabe commented Feb 29, 2016

I think we need to do a better job of commenting the tests. Without knowing mock I should be able to read through the code and get a high level understanding of what it is testing.

@surajssd
Copy link
Collaborator Author

surajssd commented Mar 1, 2016

Yes sure, its harder for somebody not knowing mock to understand what is actually going on!

@dustymabe
Copy link
Contributor

dustymabe commented Mar 1, 2016

@surajssd - rebase on upstream master so we can merge..

bonus points if you comment the tests :)

@surajssd
Wrong provider name in answers.conf, exits AtomicApp with readable error
063a59b 
When provider name in answers.conf is wrong then AtomicApp fails
with stack-trace and no error output which is user understandable.
This happened because there was no check for the condition if
given provider_key does not match with any of the providers that
are supported for now.

Fixes Issue projectatomic#562.
@surajssd surajssd force-pushed the sane-error-on-wrong-provider branch from ea12f35 to 063a59b Compare March 2, 2016 12:59
@surajssd
Copy link
Collaborator Author

surajssd commented Mar 3, 2016

@dustymabe I have added comments, was little confused about what all to include into them, share your views.

@dustymabe
Copy link
Contributor

dustymabe commented Mar 4, 2016

@cdrage can you merge this if tests pass and you are satisfied with everything?

@cdrage
Copy link
Member

cdrage commented Mar 4, 2016

Tests pass and code looks good. Thank you for the extensives tests by the way! 💃

cdrage added a commit that referenced this pull request Mar 4, 2016
@cdrage
Merge pull request #563 from surajssd/sane-error-on-wrong-provider
32bdeb1 
Wrong provider name in answers.conf, exits AtomicApp with readable error
@cdrage cdrage merged commit 32bdeb1 into projectatomic:master Mar 4, 2016
@surajssd
Copy link
Collaborator Author

surajssd commented Mar 4, 2016

☺️ thanks for the complete explanation at each point.

@dustymabe dustymabe mentioned this pull request Mar 9, 2016
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@surajssd @dustymabe @rtnpro @cdrage