Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Improved error handling #70
Improved error handling #70
Changes from 5 commits
51e654a
72c7c2c
0cc6fcd
555c61b
eca78b1
866bb8e
4fe9786
f1cbaa2
bea96f0
292ab3a
dc760f9
619d6e1
31e98d5
331182a
85dced6
2150312
593a495
196fa2a
2c043cc
24c3749
61d02ac
aba244d
3ff6fd3
23af03a
946f0ea
b666218
64f2146
7e13a74
4bca39e
ab39190
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I suggest to store the value as an
error
, because it does not forces the caller to interpret the error in a given way but leaves the freedom of printing the error the way it is actually needed.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.
done
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.
Returning
string
here would not allow the consumer to unwrap errors. Having exported the methodsFile
,LineNo
, andDocumentID
allows the consumer to craft the string representation of the error message as long as we have the original error, so I would suggest to keep the returned type aserror
.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.
Done
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.
We were discussing about returning an error here to mark that the int value should not be treated as valid. Do you think we could introduce this now?
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.
done
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.
FYI: I am not considering this function as a part of the interface of our focus, but I think, it would be nice that the NP-Guard CLI and roxctl CLI would consume the same exported interface, i.e.,
PoliciesFromFolderPath
. Maybe the functionStart
could be moved to the CLI-related pkg?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.
Will handle within the context of Issues #81 and #68
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.
When I see this kind of construct, a question arises whether it would make sense to separately expose an API for extracting connections and separately for synthesizing the policies. On the NPG page, these parts are presented graphically as separate modules, so it is a bit surprising to see that the synthesizer is actually an unexported function.
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.
To be handled in Issue #75
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.
For this function, I have mixed feelings. Here are my thoughts:
common.InArgs
, which is not transparent. As it actually uses onlyargs.DirPath
, then maybe we could simply pass a string as a parameter?To solve this, this function would need to be most probably decomposed into smaller parts and composed again. I could imagine (roughly) the following pipeline:
These functions would make the testing much easier and would allow to quickly adapt the behavior of the library to the potential future changes of the interfaces.
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.
Issues #81 and #68 should address some of the above concerns
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.
There are quite many things happening in this function and it is relatively difficult to follow the data flow. I will try to provide details inline.
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.
I think the block starting with
for _, cfgMapRef := range res.Resource.ConfigMapRefs {
could be extracted to a separate function that analyzes the resources and returns theenvs
.Moreover, I am not sure if the parse errors should be simply attached to all other errors, because the configmap has been already parsed in
r, l, c, e := parseResource(o)
, so now this must be some different kind of error.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.
In this for-block, there is a lot of repetition with the previous block. Maybe, we could extract it to a separate function that would handle both of them and extract only the differences?
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.
I feel that returning a structure with just a single field set to a value could be improved by simply returning the string, or better, an error. This is what I think about:
and as this results in a oneliner function, we could just use the
return fmt.Errorf(
instead wrapping it ingetConfigMapNotFoundError
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.
nit: Maybe we could use the
else
part instead of continuing?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.
Why don't we use
case "Deployment"
here and save thedefault
case for new unexpected types of objects?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.
Because this can be any one of the workload resources (e.g.
ReplicaSet
,Job
)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.
Would it mean that if we receive another type of k8s object that is not a deployment, we would throw an error here?
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.
The name
ScanK8sDeployObject
is a bit misleading. I'll rename it toScanK8sWorkloadObject
.Any k8s resource that is not a Service, a ConfigMap or a workload resource, shouldn't have got this far, as we already filtered the objects in
parsedK8sObjects
by their kind.Hence, an error is only issued if we fail to construct one of these objects out of its buffer.
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.
Renamed function