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

⚠️ Update Fact API to use qualified keys (source:fact) #396

Merged
merged 10 commits into from
Jun 14, 2023

Conversation

mansam
Copy link
Collaborator

@mansam mansam commented Jun 14, 2023

Get/replace/delete an anonymous fact
GET/PUT/DELETE /application/1/facts/myfact

Get/replace/delete a qualified fact
GET/PUT/DELETE /application/1/facts/mysource:myotherfact

List and replace for anonymous source:
GET/PUT /application/1/facts

List and replace all by source
GET/PUT /application/1/facts/mysource:

mansam added 3 commits June 14, 2023 01:18
Get/replace/delete an anonymous fact
GET/PUT/DELETE /application/1/facts/myfact

Get/replace/delete a qualified fact
GET/PUT/DELETE /application/1/facts/mysource:myotherfact

List and replace for anonymous source:
GET/PUT /application/1/facts

List and replace all by source
GET/PUT /application/1/facts/mysource:

Signed-off-by: Sam Lucidi <slucidi@redhat.com>
Signed-off-by: Sam Lucidi <slucidi@redhat.com>
Signed-off-by: Sam Lucidi <slucidi@redhat.com>
@mansam mansam requested a review from jortel June 14, 2023 06:02
h.FactList(ctx, source)
return
}

list := []model.Fact{}
result = h.DB(ctx).Find(&list, "ApplicationID = ? AND Key = ? AND source = ?", id, key, source)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine but generally we have been doing this in other places to reduce the raw sql.

db = h.DB(ctx)
db = db.Where("ApplicationID", id)
db = db.Where("Source", source)
db = db.Where("Key", key)

// @param id path string true "Application ID"
// @param key path string true "Fact key"
// @param source path string true "Fact source"
// @param key path string true "Qualified fact"
Copy link
Contributor

@jortel jortel Jun 14, 2023

Choose a reason for hiding this comment

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

Might be good to explain how facts are qualified in the description of each endpoint.

result = h.DB(ctx).First(&model.Fact{}, "ApplicationID = ? AND Key = ? AND source = ?", id, key, source)
if result.Error == nil {
result = h.DB(ctx).
Model(&model.Fact{}).
Where("ApplicationID = ? AND Key = ? AND source = ?", id, key, source).
Update("Value", value)
Update("Value", f.Value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the model is being fetched (and it's super small), seems like it would be simpler to:

m := &model.Fact{}
db.First(m)
m.Value = f.Value
db.Save(m)

@jortel
Copy link
Contributor

jortel commented Jun 14, 2023

I wonder if it would be good to codify the fact key format as an object to be shared by the api, binding, and addon packages. Though, could be overkill.

Something like:

type FactKey string

func (r *FactKey) With(source, name string) {
   *r = FactKey(
      strings.Join(
         []string{source, name},
         ":"))
}

func (r *FactKey) Source() (s string) {
   if x, _, found := strings.Cut(string(*r), ":"); found {
      s = x
   }
   return
}

func (r *FactKey) Name() (n string) {
   if _, x, found := strings.Cut(string(*r), ":"); found {
      n = x
   } else {
      n = string(*r)
func TestFactKey(t *testing.T) {
   g := gomega.NewGomegaWithT(t)

   key := FactKey("key")
   g.Expect(key.Source()).To(gomega.Equal(""))
   g.Expect(key.Name()).To(gomega.Equal("key"))

   key = FactKey("test:key")
   g.Expect(key.Source()).To(gomega.Equal("test"))
   g.Expect(key.Name()).To(gomega.Equal("key"))

   key = FactKey("")
   key.With("test", "key")
   g.Expect(key.Source()).To(gomega.Equal("test"))
   g.Expect(key.Name()).To(gomega.Equal("key"))
}

Signed-off-by: Sam Lucidi <slucidi@redhat.com>
mansam added 2 commits June 14, 2023 12:25
Signed-off-by: Sam Lucidi <slucidi@redhat.com>
Signed-off-by: Sam Lucidi <slucidi@redhat.com>
func (h *AppFacts) List() (facts api.FactMap, err error) {
facts = api.FactMap{}
key := api.FactKey("")
key.With(h.source, "")
Copy link
Contributor

@jortel jortel Jun 14, 2023

Choose a reason for hiding this comment

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

After seeing the usage here, I wonder if key.Qualify(source) would be more useful then With(source, name)?
Would result in usages like:

key := api.FactKey("")
key.Qualify(h.source)

And

key := api.FactKey(name)
key.Qualify(h.source)

Signed-off-by: Sam Lucidi <slucidi@redhat.com>
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.

@mansam mansam changed the title Update Fact API to use qualified keys (source:fact) ⚠️ Update Fact API to use qualified keys (source:fact) Jun 14, 2023
@@ -801,13 +778,13 @@ func (h ApplicationHandler) FactReplace(ctx *gin.Context) {
// create new Facts
if len(facts) > 0 {
newFacts := []model.Fact{}
for _, f := range facts {
value, _ := json.Marshal(f.Value)
for k, v := range facts {
Copy link
Contributor

@jortel jortel Jun 14, 2023

Choose a reason for hiding this comment

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

What if one of the keys in the map is qualified?
Should k be scrubbed through FactKey?

mansam added 3 commits June 14, 2023 15:29
Signed-off-by: Sam Lucidi <slucidi@redhat.com>
Signed-off-by: Sam Lucidi <slucidi@redhat.com>
Signed-off-by: Sam Lucidi <slucidi@redhat.com>
@mansam mansam merged commit 02f4452 into konveyor:main Jun 14, 2023
@mansam mansam mentioned this pull request Jun 14, 2023
ibolton336 added a commit to konveyor/tackle2-ui that referenced this pull request Jun 22, 2023
- Adds initial implementation of the facts modal, analysis detail drawer
tab for facts, and code viewer for facts json.

Notes: This does not cover the filter/sort logic for facts based on
sources. There is no mock data available from the api for this yet, but
we will be required to collate the fact sources ourselves on the UI side
based on the fact keys returned from the hub.
konveyor/tackle2-hub#396 for more info on how
the api will look.

TODO: #1061 
TODO: [How should we type facts with an unknown data type?
](#1062)

---------

Signed-off-by: ibolton336 <ibolton@redhat.com>
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