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

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

Merged
merged 2 commits into from
Nov 1, 2024

Conversation

sasirven
Copy link
Contributor

@sasirven sasirven commented Jul 26, 2024

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.

@elegantmoose
Copy link
Contributor

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

@uruwhy
Copy link
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
Contributor Author

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
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
Contributor Author

sasirven 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?

@uruwhy
Copy link
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).

app/objects/c_operation.py Outdated Show resolved Hide resolved
@sasirven sasirven force-pushed the feature/update-api-for-schedule branch from 6bf8591 to 8036099 Compare October 15, 2024 09:07
Copy link
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

Copy link
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
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.
- 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
@elegantmoose
Copy link
Contributor

Thank you @sasirven !

@elegantmoose elegantmoose merged commit 4039949 into mitre:master Nov 1, 2024
1 of 6 checks passed
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.

4 participants