Skip to content

Feature - Update API for schedule (2/3)#3026

Merged
mkultraWasHere merged 2 commits into
apache:masterfrom
sasirven:feature/update-api-for-schedule
Nov 1, 2024
Merged

Feature - Update API for schedule (2/3)#3026
mkultraWasHere merged 2 commits into
apache:masterfrom
sasirven:feature/update-api-for-schedule

Conversation

@sasirven
Copy link
Copy Markdown
Contributor

@sasirven sasirven commented Jul 26, 2024 •
edited
Loading

This is the API part of the schedule management with cron, which depends on #3025

Linked Merge Requests:
#3025
mitre/magma#61

  • Update the operation schema:
    • Make the obfuscator required.
  • Update the schedule schema:
    • Add some metadata to the schedule field for Swagger documentation. (The string type and required attribute are from the 1st Pull Request)
    • Add a cron validation for the schedule field to ensure it is a valid standard cron expression.
  • Fix the operation creation:
    • Manually change the UUID to ensure the creation of a new operation. (the deepcopy also copies the existing UUID)
    • Add the creation date to the name for better readability.

This was referenced Jul 26, 2024
Merged
Merged
@mkultraWasHere
Copy link
Copy Markdown
Contributor

mkultraWasHere commented Sep 5, 2024

@sasirven we are picking this up now. Apologies for delay.

@uruwhy
Copy link
Copy Markdown
Contributor

uruwhy commented Sep 6, 2024

Out of curiosity, what is the motivation behind making the obfuscator required rather than using the default value?

@sasirven sasirven force-pushed the feature/update-api-for-schedule branch from 840f755 to 6bf8591 Compare September 24, 2024 11:32
@sasirven
Copy link
Copy Markdown
Contributor Author

sasirven commented Sep 25, 2024

Out of curiosity, what is the motivation behind making the obfuscator required rather than using the default value?

As far as I remember, the default value is “plain-text,” but I’m not sure if this default value is directly available in the API. We need to add a plugin, that's right?
By making the obfuscator required in the API, it would allow the user to use the API without the GUI and potentially have another plugin that does not have the "plain-text" value.

@uruwhy
Copy link
Copy Markdown
Contributor

uruwhy commented Sep 26, 2024

Out of curiosity, what is the motivation behind making the obfuscator required rather than using the default value?

As far as I remember, the default value is “plain-text,” but I’m not sure if this default value is directly available in the API. We need to add a plugin, that's right? By making the obfuscator required in the API, it would allow the user to use the API without the GUI and potentially have another plugin that does not have the "plain-text" value.

plain-text is the default obfuscator and is provided by the stockpile plugin, which is installed and enabled out-of-the-box and is used in the vast majority of scenarios. I imagine this change would address the edge case where a user does not have the stockpile plugin enabled for whatever reason, and thus would have to specify an obfuscator to avoid generating errors. On the other hand, making the obfuscator required 100% of the time would disrupt users that currently do not specify one in order to leverage the default value. I'm personally leaning more towards keeping it as-is (not required), since anyone who currently uses the operations/schedule API but does not use the stockpile plugin would have to specify the obfuscator anyway, and they would already be aware of that.

@clenk @elegantmoose any additional thoughts?

@sasirven
Copy link
Copy Markdown
Contributor Author

sasirven commented Sep 26, 2024 •
edited
Loading

Out of curiosity, what is the motivation behind making the obfuscator required rather than using the default value?

As far as I remember, the default value is “plain-text,” but I’m not sure if this default value is directly available in the API. We need to add a plugin, that's right? By making the obfuscator required in the API, it would allow the user to use the API without the GUI and potentially have another plugin that does not have the "plain-text" value.

plain-text is the default obfuscator and is provided by the stockpile plugin, which is installed and enabled out-of-the-box and is used in the vast majority of scenarios. I imagine this change would address the edge case where a user does not have the stockpile plugin enabled for whatever reason, and thus would have to specify an obfuscator to avoid generating errors. On the other hand, making the obfuscator required 100% of the time would disrupt users that currently do not specify one in order to leverage the default value. I'm personally leaning more towards keeping it as-is (not required), since anyone who currently uses the operations/schedule API but does not use the stockpile plugin would have to specify the obfuscator anyway, and they would already be aware of that.

@clenk @elegantmoose any additional thoughts?

This leads me to another question:
If a user does not use the default value and makes a direct call to the API, will the obfuscator later be set with the default value, i.e., plain-text? Or are you only referring to the GUI that sets the default value?
Because in this case, technically, the GUI will fill the obfuscator with the default value right?

@uruwhy
Copy link
Copy Markdown
Contributor

uruwhy commented Sep 26, 2024

Out of curiosity, what is the motivation behind making the obfuscator required rather than using the default value?

As far as I remember, the default value is “plain-text,” but I’m not sure if this default value is directly available in the API. We need to add a plugin, that's right? By making the obfuscator required in the API, it would allow the user to use the API without the GUI and potentially have another plugin that does not have the "plain-text" value.

plain-text is the default obfuscator and is provided by the stockpile plugin, which is installed and enabled out-of-the-box and is used in the vast majority of scenarios. I imagine this change would address the edge case where a user does not have the stockpile plugin enabled for whatever reason, and thus would have to specify an obfuscator to avoid generating errors. On the other hand, making the obfuscator required 100% of the time would disrupt users that currently do not specify one in order to leverage the default value. I'm personally leaning more towards keeping it as-is (not required), since anyone who currently uses the operations/schedule API but does not use the stockpile plugin would have to specify the obfuscator anyway, and they would already be aware of that.
@clenk @elegantmoose any additional thoughts?

This leads me to another question: If a user does not use the default value and makes a direct call to the API, will the obfuscator later be set with the default value, i.e., plain-text? Or are you only referring to the GUI that sets the default value? Because in this case, technically, the GUI will fill the obfuscator with the default value right?

The operation constructor uses plain-text as the default obfuscator value. So if you use the API to create an operation and don't specify the obfuscator, it will default to that value (which is how I created operations to test the scheduler in updates in #3025).

uruwhy
uruwhy reviewed Oct 2, 2024
View reviewed changes
Comment thread app/objects/c_operation.py Outdated
uruwhy
uruwhy reviewed Oct 2, 2024
View reviewed changes
Comment thread app/objects/c_schedule.py
@sasirven sasirven force-pushed the feature/update-api-for-schedule branch from 6bf8591 to 8036099 Compare October 15, 2024 09:07
uruwhy
uruwhy approved these changes Oct 22, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@uruwhy uruwhy left a comment

Choose a reason for hiding this comment

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

unit tests need to be fixed to adhere to new format checks

uruwhy
uruwhy suggested changes Oct 22, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@uruwhy uruwhy left a comment

Choose a reason for hiding this comment

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

unit tests need to be fixed to adhere to new format checks

jbaptperez and others added 2 commits October 28, 2024 12:43
@jbaptperez @sasirven
Creates a new operation per schedule
d7e2d09 
When an object schedule is created, it embeds a complete operation
template in its "task" field.
On the first schedule time, it creates an operation from the template.
Until now, this operation was used for each subsequent schedule.
This commit prevent that and creates a new operation per schedule, with
the start date added in its name.
@sasirven
Update schedule Schemas
6898392 
- Add a validation step to ensure the schedule field has a valid cron
  syntax.
- Update the tests
@sasirven sasirven force-pushed the feature/update-api-for-schedule branch from f8aed5b to 6898392 Compare October 28, 2024 11:45
@mkultraWasHere
Copy link
Copy Markdown
Contributor

mkultraWasHere commented Nov 1, 2024

Thank you @sasirven !

@mkultraWasHere mkultraWasHere merged commit 4039949 into apache:master Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@mkultraWasHere mkultraWasHere mkultraWasHere approved these changes

@uruwhy uruwhy uruwhy approved these changes

Reviewers whose approvals may not affect merge requirements

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

@sasirven @mkultraWasHere @uruwhy @jbaptperez