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

feat: add workflow triggers #439

Merged
merged 9 commits into from
Oct 25, 2024
Merged

feat: add workflow triggers #439

merged 9 commits into from
Oct 25, 2024

Conversation

nodegard
Copy link
Contributor

Description

Please describe the change you have made.

Checklist:

Copy link

codecov bot commented Oct 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 42.92%. Comparing base (a2a5934) to head (81c59ff).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #439   +/-   ##
=======================================
  Coverage   42.91%   42.92%           
=======================================
  Files         497      497           
  Lines       36747    36747           
=======================================
+ Hits        15771    15772    +1     
+ Misses      20976    20975    -1     

see 1 file with indirect coverage changes

eriklien
eriklien previously approved these changes Oct 24, 2024
Copy link
Contributor

@eriklien eriklien left a comment

Choose a reason for hiding this comment

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

Looks good. Added a suggestion and a comment.

externalId: "bid_configuration_day_ahead_water_value_no2_full"
metadata:
config: "bid_configuration_day_ahead_water_value_no2_full"
market: "{{wf_day_ahead_market}}"
Copy link
Contributor

Choose a reason for hiding this comment

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

comment: What do we need this for? Front-end, I guess? Not really related to this PR, but I would think this is redundant information, unless this is what the workflow uses (finding all workflow executions with metadata.market="DAY_AHEAD") to determine that this is a workflow execution supposed to produce a day-ahead bid? The combination of method, price area and config also seems redundant, given that the config contains that information (though it may require another API call). Probably best to discuss this with @notrixbe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea for the front-end not an ideal solution, but @notrixbe and I have aligned on it

power_ops_type_space: "power_ops_types"
power_ops_instance_space: "power_ops_instances"
power_ops_models_space: "power_ops_core"
version: "1"
power_ops_version: "1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
power_ops_version: "1"
power_ops_data_model_version: "1"

Suggestion to avoid confusion with e. g. PowerOps SDK version

toolkit/cdf.toml Show resolved Hide resolved
@nodegard nodegard merged commit 29998c1 into main Oct 25, 2024
7 checks passed
@nodegard nodegard deleted the feat/add-workflow-trigger branch October 25, 2024 13:14
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