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

Msalem/fix scheduler button #570

Closed
wants to merge 13 commits into from

Conversation

msalemcode
Copy link
Contributor

@msalemcode msalemcode commented Oct 12, 2023

Enforce the scheduler code to check if the feature is enabled.

Test the PR:
1- Enable the metered support flag. Make sure you can access all pages for scheduler including the link from Subscription Managed Usage

2- Disable the metered support flag. Make sure you can not see or access the any scheduler page

3- try hardcoded url <>/scheduler <>/scheduler/NewScheduler ..etc

4- Enable again and see if you can access the pages

Any other use case please let me know.

@santhoshb-msft santhoshb-msft self-requested a review October 13, 2023 17:04
@santhoshb-msft santhoshb-msft self-assigned this Oct 13, 2023
@msalemcode msalemcode linked an issue Oct 17, 2023 that may be closed by this pull request
Copy link
Contributor

@santhoshbomma9 santhoshbomma9 left a comment

Choose a reason for hiding this comment

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

@msalemcode I turned on the metered scheduler, created an item and turned off. But looks like my meters are still being reported. I think this might a major functionality gap. hence assigning the PR back to you. Please let me know if you want to discuss further.

Copy link
Contributor

@santhoshb-msft santhoshb-msft left a comment

Choose a reason for hiding this comment

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

@msalemcode I turned on the metered scheduler, created an item and turned off. But looks like my meters are still being reported. I think this might a major functionality gap. hence assigning the PR back to you. Please let me know if you want to discuss further.

src/AdminSite/Controllers/SchedulerController.cs Outdated Show resolved Hide resolved
src/DataAccess/Migrations/Custom/BaselineV7_Seed.cs Outdated Show resolved Hide resolved
src/AdminSite/Controllers/ApplicationConfigController.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@santhoshb-msft santhoshb-msft left a comment

Choose a reason for hiding this comment

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

Changes observed:

  1. doc updates with new way of enabling meter scheduler using db setting
  2. added appconfigservice into the base controller and set the logic to enable/disable meter scheduler
  3. passed the appconfig service from all controllers
  4. adjusted to show/hide scheduler controller actions based on 2 and other views
  5. baseline 741 seed/dessed added for new db setting
  6. meter scheduler logic updated based on the dbsetting
  7. Tested directed deploy and upgrade scenario,

@msalemcode all looks good except for one step. Lets say 7.4.0 is in use with active scheduler items and someone upgraded to this release and the db setting is by default disabled, which means all the existing scheduler items wont run right. We should go in manually and update the db setting. How should we handle this?
8.
9.
10.

@santhoshb-msft
Copy link
Contributor

santhoshb-msft commented Dec 7, 2023

Added installer message that the scheduler is enabled on upgrade if the version upgrade is > 7.4.0
Default the value to "true"

Copy link
Contributor

@santhoshb-msft santhoshb-msft left a comment

Choose a reason for hiding this comment

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

@msalemcode could you validate the below scenario please?

  1. deploy 7.4.0
  2. create hourly schedule
  3. let it run once
  4. upgrade to your new PR code
  5. Expected, hourly job should run by default
  6. Actual: Item didnt get picked up

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.

User can schedule task even if scheduler is disabled
3 participants