Closed shivam-Purohit closed 10 months ago
Suggest if further changes are required.
@SarthakJain26 @Saranya-jena can I get a review?
@shivam-Purohit can you please also share the screen shots of the command and the output
@shivam-Purohit please add these commands in the readme and usage docs as well
@shivam-Purohit can you please also share the screen shots of the command and the output
sure!
@shivam-Purohit please add these commands in the readme and usage docs as well
I added these in 0.23.0. Do we need to add in any other versions? i think we do not need that.
@SarthakJain26 I fixed all the changes you requested. Let me know if further changes are required.
@shivam-Purohit , you have displayed the complete response coming from the API which is not a very good user experience. You will have to extract required fields and you can display them in table format. I have attached the screenshot for reference. You can display environmentID, name, createdAt and createdBy fields. Please update the PR accordingly
hey @SarthakJain26 I am currently getting the response as null in many fields which ig should not. I tried rechecking the API. Is there something I am doing here to retrieve the data?
hey @SarthakJain26 I am currently getting the response as null in many fields which ig should not. I tried rechecking the API. Is there something I am doing here to retrieve the data?
@shivam-Purohit Possibly yes. You need to check on what is the graphql request you are sending and then what is the data you are expecting back. While decoding the response from the graphql server you need to check if the struct you are using to decode the response has the required fields or not.
@SarthakJain26 the error was actually due to query itself which we were sending through api. I changed it a bit for the newcommand. I also refactored the output and reused some other components for similarity and reusability.
Thanks @shivam-Purohit, update output looks awesome
@SarthakJain26, do we need two separate commands for this, i.e., list and get? Maybe get
should be enough only to show a list of all environments, as that is what we're doing for experiments and infra.
@SarthakJain26, do we need two separate commands for this, i.e., list and get? Maybe
get
should be enough only to show a list of all environments, as that is what we're doing for experiments and infra.
@Nageshbansal , by convention, list command can be used to list the resources under the project with required info, and get command can be used to get additional info about a particular resource. We can follow the same in other places as well. Adding to this @shivam-Purohit please let us know if you can add other details also in the get environment command in couple of days so that we can merge this PR and take it in this release.
@SarthakJain26, do we need two separate commands for this, i.e., list and get? Maybe
get
should be enough only to show a list of all environments, as that is what we're doing for experiments and infra.@Nageshbansal , by convention, list command can be used to list the resources under the project with required info, and get command can be used to get additional info about a particular resource. We can follow the same in other places as well. Adding to this @shivam-Purohit please let us know if you can add other details also in the get environment command in couple of days so that we can merge this PR and take it in this release.
type Environment struct {
ProjectID string `json:"projectID"`
EnvironmentID string `json:"environmentID"`
Name string `json:"name"`
Description *string `json:"description"`
Tags []string `json:"tags"`
Type EnvironmentType `json:"type"`
CreatedAt string `json:"createdAt"`
CreatedBy *UserDetails `json:"createdBy"`
UpdatedBy *UserDetails `json:"updatedBy"`
UpdatedAt string `json:"updatedAt"`
IsRemoved *bool `json:"isRemoved"`
InfraIDs []string `json:"infraIDs"`
}
these are the details we can get from the response. @SarthakJain26 let me know what details you want to add, imo we can add type, description or maybe updated at and updated by.
@shivam-Purohit you can also add the InfraIDs along with what you mentioned so that it infras within the environment can be linked.
@shivam-Purohit you can also add the InfraIDs along with what you mentioned so that it infras within the environment can be linked.
sure!
i also added the environment type after the screenshot was taken. Switched the structure to row like as it was overflowing and difficult to manage in table format. Hope this helps @SarthakJain26
@shivam-Purohit can you please fix the fmt issues due to which build pipeline is failing and also sign your commits so that DCO can pass. We can then merge the PR
can you please fix the fmt issues due to which build pipeline is failing and also sign your commits so that DCO can pass. We can then merge the PR
should I squash the commits into one?
@shivam-Purohit Squashing the commit is not required ( it will be taken care of while merging) , for now please fix the fmt in go files ( you can use gofmt ./...
command to format the go files)
@Nageshbansal my first commit was unsigned i could not find any other way to sign it. So I merged it. If the workflows get approval i can test the changes i just made . I use gofmt -w . that should be able to get rid of the fmt error
@shivam-Purohit can you please fix the fmt issues due to which build pipeline is failing and also sign your commits so that DCO can pass. We can then merge the PR
@SarthakJain26 can you run the workflow again ig this should be fine now at least the fmt error
Please make the changes suggested by @Jonsy13 . Rest all looks good. 🚀
@Jonsy13 @Saranya-jena I made the changes suggested you can take a look. Suggest if any further changes needs to be done.
@shivam-Purohit please fix the build pipeline
@SarthakJain26 fixed the fmt error. Hope it resolves the build workflow
Looks like it worked!
refers issue #164
Addition of new commands :