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

chore: add validity check for logging #161

Merged
merged 8 commits into from
Oct 11, 2024

Conversation

renaudjester
Copy link
Collaborator

Fix CMT-110

Allow user to check the validity of their credentials

@uriii3 uriii3 mentioned this pull request Oct 10, 2024
@renaudjester renaudjester requested a review from uriii3 October 10, 2024 15:12
tests/test_login.py Outdated Show resolved Hide resolved
tests/test_login.py Outdated Show resolved Hide resolved
Comment on lines +260 to +277
command = [
"copernicusmarine",
"login",
"--username",
f"{os.getenv('COPERNICUSMARINE_SERVICE_USERNAME')}",
"--password",
f"{os.getenv('COPERNICUSMARINE_SERVICE_PASSWORD')}",
"--configuration-file-directory",
f"{non_existing_directory}",
]

self.output = execute_in_terminal(command)

environment_without_crendentials = (
get_environment_without_crendentials()
)

command = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are we rewriting the definition of the command?

Copy link
Collaborator

Choose a reason for hiding this comment

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

and we are using the same 'self' then to assert... makes for a weird test, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because in python you can :D the object is just overwritten and since we don't need both at the same time, why not?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean we don't need both but if we overwrite the self.output we destroy the test, no? (or is it appended and then my question makes no sense?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What do you mean we destroy the test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And no it's not appended, but in the whole test suit we do: self.output =

Comment on lines +310 to +318
assert (folder / ".copernicusmarine-credentials").is_file()
assert login(
check_credentials_valid=True,
)

assert not login(
username="toto",
password="tutu",
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't you add a test specific also for the option that you created? For the python interface a little more extensive? Or with the cli tests it will be enough?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Basically the CLI and the python interface sends to the same function to I am checking that the arguments work

@renaudjester renaudjester merged commit 355cc1b into copernicusmarine-toolbox-v2 Oct 11, 2024
2 checks passed
@renaudjester renaudjester deleted the check-validity-login branch October 11, 2024 15:56
renaudjester added a commit that referenced this pull request Oct 28, 2024
Add a functionality to check if the user's credentials are valid. 
Deletes the `--skip-if-logged-in` functionality
renaudjester added a commit that referenced this pull request Oct 28, 2024
Add a functionality to check if the user's credentials are valid. 
Deletes the `--skip-if-logged-in` functionality
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