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

#33 Added configurable idle time for a SaaS database #36

Merged
merged 4 commits into from
Jul 31, 2024

Conversation

ahsimb
Copy link
Contributor

@ahsimb ahsimb commented Jul 30, 2024

closes #33

@ahsimb ahsimb added the feature Product feature label Jul 30, 2024
@ahsimb ahsimb requested a review from ckunki July 30, 2024 15:27
@ahsimb ahsimb self-assigned this Jul 30, 2024
parser.addoption(
"--idle-time",
action="store",
default="2",
Copy link

Choose a reason for hiding this comment

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

This mean we could have a minimum idle time of 1h. Couldn't there be cases where we want to have a shorter idle-time, maybe 30min or so?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Firstly, it's a float, 30 min is 0.5.
Secondly, I don't see cases when we need to set it to a shot period. The database fixture is supposed to shut down the database as soon as all tests are done. Unless we want to keep the database to investigate the effect of our tests. In that case, we probably want to give ourselves enough time for investigation and then shut down the db manually.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering about whether we should rename the CLI option.
I think --idle-time is very unspecific, it should say something about the semantics of the idle duration:

  • which component is affected, in this case is clear I think: the SaaS databse instance
  • what will happen after the timeout, i.e. an autostop
  • especially the unit should be clear, AFAIK @ahsimb chose hours which also deviates from the unit minutes in the original SaaS API.

In saas-api-python/.../init.py the constant is called AUTOSTOP_DEFAULT_IDLE_TIME with datatype timedelta.

Copy link
Contributor

Choose a reason for hiding this comment

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

Proposal:

  • change to unit minutes
  • use name --autostop-idle-minutes for CLI option

ckunki
ckunki previously requested changes Jul 31, 2024
parser.addoption(
"--idle-time",
action="store",
default="2",
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering about whether we should rename the CLI option.
I think --idle-time is very unspecific, it should say something about the semantics of the idle duration:

  • which component is affected, in this case is clear I think: the SaaS databse instance
  • what will happen after the timeout, i.e. an autostop
  • especially the unit should be clear, AFAIK @ahsimb chose hours which also deviates from the unit minutes in the original SaaS API.

In saas-api-python/.../init.py the constant is called AUTOSTOP_DEFAULT_IDLE_TIME with datatype timedelta.

@ahsimb
Copy link
Contributor Author

ahsimb commented Jul 31, 2024

@ckunki, how about
--saas-timeout
or
--saas-idle-hours

Hours are better than minutes (which, in turn, better than seconds and milliseconds).

@ahsimb
Copy link
Contributor Author

ahsimb commented Jul 31, 2024

OK, the ticket got more complicated than originally thought. Back to the drawing board.

@ahsimb
Copy link
Contributor Author

ahsimb commented Jul 31, 2024

Please ignore my previous comment. I decided to do the follow-up development in a separate PR.

@ahsimb ahsimb requested review from tomuben and ckunki July 31, 2024 12:26
pytest-saas/doc/changes/unreleased.md Show resolved Hide resolved
parser.addoption(
"--idle-time",
action="store",
default="2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Proposal:

  • change to unit minutes
  • use name --autostop-idle-minutes for CLI option

@ahsimb ahsimb dismissed ckunki’s stale review July 31, 2024 12:48

ckunki has approved the PR

@ahsimb ahsimb merged commit 3637087 into main Jul 31, 2024
1 check passed
@ahsimb ahsimb deleted the feature/33-configurable-idle-time branch July 31, 2024 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Product feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the saas_database fixture configurable in terms of the idle time
3 participants