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
🌱 add dependency package in test/api #375
🌱 add dependency package in test/api #375
Changes from 2 commits
6ed6cd0
06e448e
a8449a5
a07ee95
5ad6f23
3f4253c
08ad217
cb13080
890a794
16efa1c
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.
The API Dependency links two Applications and keeps their IDs in From/To fields. These fields are Refs (https://github.com/konveyor/tackle2-hub/blob/main/api/base.go#L379) so contain just IDs (and optionally Name, not that not important), but the Refs are not actual Applications. The From and To Applications need to be created before creating Dependency.
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.
As I understood from the review that dependency need To/From fields which will be used to refer to the applications?
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.
and that To/From will be generated by creating an instance of application in
api_test.go
. Am i in the right direction?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.
Yes
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.
Same with creating Dependencies - 1st Applications, 2nd Dependency between them.
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 should be a Dependency creation and condition below should check if
got
is an array with the created dependency. Sorry, I missed this part before.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.
Shall I just compare the ID's of dependency created and the ApplicationFrom/To ID's will it suffice?
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.
Some of these lines seems to be redundant to me (the dependency here and init() below). But I'd suggest get it to (partially) working state (passing tests against locally running Hub https://github.com/konveyor/tackle2-hub/tree/main/test#rest-api) and we can refactor it then.
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.
Refactored the code Kindly review
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.
Definition of Dependencies (and likely also TestCases) will need to be updated. There is some limitation caused by api.Dependency keeping just Ref of Aplication, so we need store application elsewhere. I can see few options:
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 1st option is easier to implement because it involves less complexities and 2nd option is is less intuitive which can be easily dealt by the 1st. What's your say on that?
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 have a thought process of injecting dependency withdependecy-less TestCase in Samples[]. and then use From/To in the
api_test.go
to access them in thedependency
to be used by various CRUD functions.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'm not sure I fully understand, but sounds good.
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.
A Followup to this discussion and the 1st option, the dependency variable (and Samples containing only the dependency when api_test.go creates new applicationFrom/To instances) here seems to me to be overcomplicated.
What about drop the dependency variable, considering update/rename TestCase and putting to Samples (or renamed array for tests) and doing all this in since var( ... ) block?