-
Notifications
You must be signed in to change notification settings - Fork 35
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
Conversation
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>
api/application.go
Outdated
h.FactList(ctx, source) | ||
return | ||
} | ||
|
||
list := []model.Fact{} | ||
result = h.DB(ctx).Find(&list, "ApplicationID = ? AND Key = ? AND source = ?", id, key, source) |
There was a problem hiding this comment.
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)
api/application.go
Outdated
// @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" |
There was a problem hiding this comment.
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.
api/application.go
Outdated
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) |
There was a problem hiding this comment.
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)
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:
|
Signed-off-by: Sam Lucidi <slucidi@redhat.com>
Signed-off-by: Sam Lucidi <slucidi@redhat.com>
Signed-off-by: Sam Lucidi <slucidi@redhat.com>
addon/application.go
Outdated
func (h *AppFacts) List() (facts api.FactMap, err error) { | ||
facts = api.FactMap{} | ||
key := api.FactKey("") | ||
key.With(h.source, "") |
There was a problem hiding this comment.
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great.
@@ -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 { |
There was a problem hiding this comment.
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?
Signed-off-by: Sam Lucidi <slucidi@redhat.com>
Signed-off-by: Sam Lucidi <slucidi@redhat.com>
Signed-off-by: Sam Lucidi <slucidi@redhat.com>
- 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>
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: