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

⚠️ Separate Target and RuleSet models/apis #464

Merged
merged 6 commits into from
Aug 3, 2023

Conversation

mansam
Copy link
Collaborator

@mansam mansam commented Jul 28, 2023

No description provided.

api/target.go Outdated

func (h TargetHandler) AddRoutes(e *gin.Engine) {
routeGroup := e.Group("/")
routeGroup.Use(Required("rulesets"), Transaction)
Copy link
Contributor

@jortel jortel Jul 31, 2023

Choose a reason for hiding this comment

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

s/rulesets/targets
Also, needs to be added to the realm.

Signed-off-by: Sam Lucidi <[email protected]>
Signed-off-by: Sam Lucidi <[email protected]>
Signed-off-by: Sam Lucidi <[email protected]>
@mansam mansam marked this pull request as ready for review August 2, 2023 15:07
@@ -268,13 +309,10 @@ func (r *RuleSet) With(m *model.RuleSet) {
// Model builds a model.
func (r *RuleSet) Model() (m *model.RuleSet) {
m = &model.RuleSet{
Kind: r.Kind,
Name: r.Name,
Description: r.Description,
Copy link
Contributor

Choose a reason for hiding this comment

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

Long term we may want RuleSets to have an optional description. What do you think about leaving it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, good idea.


func (h TargetHandler) AddRoutes(e *gin.Engine) {
routeGroup := e.Group("/")
routeGroup.Use(Required("targets"), Transaction)
Copy link
Contributor

@jortel jortel Aug 2, 2023

Choose a reason for hiding this comment

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

This is probably fine and practical but expected the Transaction only on:
Create
Update
Delete

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we've been using it like this everywhere, and relying on the method check in the Transaction handler unless I'm mistaken.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're correct. I forgot.

Name string `gorm:"uniqueIndex;not null"`
Description string
Choice bool
Labels JSON
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing: gorm:"type:json"

Copy link
Contributor

@jortel jortel left a comment

Choose a reason for hiding this comment

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

Looks great!


filter, err := qf.New(ctx,
[]qf.Assert{
{Field: "labels", Kind: qf.STRING},
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding name to the filter in the interest of completeness may be a nice to have but can always add later.

Signed-off-by: Sam Lucidi <[email protected]>
@mansam mansam merged commit 0d0355b into konveyor:main Aug 3, 2023
8 of 10 checks passed
aufi added a commit to aufi/tackle2-hub that referenced this pull request Aug 3, 2023
Image is not part of Ruleset anymore, removing from sample Rulesets.
Related to konveyor#464

Signed-off-by: Marek Aufart <[email protected]>
jortel pushed a commit that referenced this pull request Aug 3, 2023
Image is not part of Ruleset anymore, removing from sample Rulesets.

Related to #464

Signed-off-by: Marek Aufart <[email protected]>
ibolton336 added a commit to konveyor/tackle2-ui that referenced this pull request Aug 7, 2023
UI changes relating to konveyor/tackle2-hub#464

- Rename setting for ruleset/target order to match api changes
- Change TS model for Ruleset to match api changes. Ruleset is now a
field on the target model and can be appended to directly / read from
directly when performing CRUD operations against a custom target.
- Drive target card selection from label rather than ruleset/target
since we no longer need to store the entire ruleset/target state in the
wizard. This is because the analyzer no longer requires it to be passed
in the TaskData. We are now only passing labels. This simplifies the
wizard state & allows tracking of individual ruleset selection/ source
selection/ target selection to be removed.
- Now we can drive the select targets / select sources dropdowns on the
set-options analysis wizard form from labels rather than tracking
specific state for either of those.
- Changes format of default sources/targets to match new hub api label
format

---------

Signed-off-by: ibolton336 <[email protected]>
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