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

Initializing new Team Configuration File #166

Merged
merged 23 commits into from
Dec 18, 2024
Merged

Conversation

chinmdas
Copy link
Contributor

@chinmdas chinmdas commented Dec 13, 2024

Initializing new Team Configuration File and remove the obsolete web page,

What It Does

  • No more obsolete web page when user tries to create a new CICS Profile. Instead a new global team config file will get initialized.

How to Test

  • If NO global zowe.config.json file exists: Creates a New.
  • If zowe.config.json file exists, but NO CICS Profile is present: Gives option to Edit.
  • If zowe.config.json file exists and CICS Profile is available: Gives option to Edit.

Review Checklist
I certify that I have:

Additional Comments

Initializing new Team Configuration File and remove the obsolete web page,

Signed-off-by: Chinmay Das <[email protected]>
Signed-off-by: Chinmay Das <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Dec 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.81%. Comparing base (acfe17a) to head (c01d2b6).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #166   +/-   ##
=======================================
  Coverage   93.81%   93.81%           
=======================================
  Files          75       75           
  Lines         776      776           
  Branches       45       45           
=======================================
  Hits          728      728           
  Misses         48       48           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@chinmdas
Copy link
Contributor Author

This PR fixes #33.

Copy link
Contributor

@AndrewTwydell AndrewTwydell left a comment

Choose a reason for hiding this comment

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

I get this popup when clicking "add" with no config file. We should change that text to something more meaningful.

image

Copy link
Contributor

@AndrewTwydell AndrewTwydell left a comment

Choose a reason for hiding this comment

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

When clicking edit config file when there are no profiles in there, we get a horrible error
image

image

@AndrewTwydell
Copy link
Contributor

When you click the Add button on other ZE views and no config file exists, you get this menu
image

Perhaps we should do the same to be consistent, rather than going straight into the Global or Project quickpick??

@Joe-Winchester Joe-Winchester self-requested a review December 13, 2024 15:47
@Joe-Winchester
Copy link
Member

Hi @AndrewTwydell ,

Where did you get a .vsix from to do the tests for the screen shots you've shown above ? I did a code review which looked good but I'd like to do a test of the PR code. Could checkout the branch and launch from VS code launch.json but wondering if there is a .vsix build created for the PR ?

@chinmdas
Copy link
Contributor Author

I get this popup when clicking "add" with no config file. We should change that text to something more meaningful.

image

Yes, Agreed..!!! What can be appropriate info message? Do you think this makes sense?
Team Configuration file not found. Creating new Team Configuration file

@chinmdas
Copy link
Contributor Author

When clicking edit config file when there are no profiles in there, we get a horrible error image

image

Nice Testing @AndrewTwydell ..!!
The logic of picking up filepath for showQuickPick is very tightly coupled with profiles availability in zowe.config.json file. Since, you have delete all the profiles entry, extraction of filepath broke.
I am going to fix this. Thanks..!!

@chinmdas
Copy link
Contributor Author

When you click the Add button on other ZE views and no config file exists, you get this menu image

Perhaps we should do the same to be consistent, rather than going straight into the Global or Project quickpick??

Agreed...!!! Will implement this...!!

@chinmdas
Copy link
Contributor Author

Hi @AndrewTwydell ,

Where did you get a .vsix from to do the tests for the screen shots you've shown above ? I did a code review which looked good but I'd like to do a test of the PR code. Could checkout the branch and launch from VS code launch.json but wondering if there is a .vsix build created for the PR ?

Thanks @Joe-Winchester .... I see all the valid points raised by @AndrewTwydell . I would implement his suggestions and seek your approval again..!!

@Joe-Winchester
Copy link
Member

Joe-Winchester commented Dec 13, 2024

On the CICS profile I still see the pop-up action Update Profile that opens the Connection Details dialog.
The connection details dialog should be removed so it's not accessible at all (maybe just yank all the code), and instead Update Profile should probably drop into the editor for the zowe.config.json file itself.
(Note, this testing was me checking out the branch and doing a debug launch so it's possible I have a false test, or also possible this fix is outside the scope of the issue this PR is fixing, and maybe another issue should be raised for removing the deprecated page for profile editing alltogether.

image

@chinmdas
Copy link
Contributor Author

On the CICS profile I still see the pop-up action Update Profile that opens the Connection Details dialog. The connection details dialog should be removed so it's not accessible at all (maybe just yank all the code), and instead Update Profile should probably drop into the editor for the zowe.config.json file itself. (Note, this testing was me checking out the branch and doing a debug launch so it's possible I have a false test, or also possible this fix is outside the scope of the issue this PR is fixing, and maybe another issue should be raised for removing the deprecated page for profile editing alltogether.

image

Okay.... Thank you bringing out this scenario @Joe-Winchester . I will fix this as well.

packages/vsce/src/trees/CICSTree.ts Fixed Show fixed Hide fixed
packages/vsce/src/trees/CICSTree.ts Fixed Show fixed Hide fixed
packages/vsce/src/trees/CICSTree.ts Fixed Show fixed Hide fixed
@davenice
Copy link
Contributor

I think this looks pretty good - can you squash the commits down to 1 because it's just one piece of work?

That'll fix the DCO issue and make sure that whenever it gets merged it looks like one seamless piece? :-D

@chinmdas
Copy link
Contributor Author

enever it gets merged it looks like one seamless piece

I dont know, if I get 2 approvals, I can check if I have an option of squash merge.

@zFernand0
Copy link
Member

no need to squash commit now 🙏

You should be able to follow the preferred approach here:

image

…may Das <[email protected]>, hereby add my Signed-off-by to this commit: 1fd891c I, Chinmay Das <[email protected]>, hereby add my Signed-off-by to this commit: 761aa07 I, Chinmay Das <[email protected]>, hereby add my Signed-off-by to this commit: a03249b  Signed-off-by: Chinmay Das <[email protected]>

DCO Remediation Commit for Chinmay Das <[email protected]>

I, Chinmay Das <[email protected]>, hereby add my Signed-off-by to this commit: 1fd891c
I, Chinmay Das <[email protected]>, hereby add my Signed-off-by to this commit: 761aa07
I, Chinmay Das <[email protected]>, hereby add my Signed-off-by to this commit: a03249b

Signed-off-by: Chinmay Das <[email protected]>

Signed-off-by: Chinmay Das <[email protected]>
@AndrewTwydell
Copy link
Contributor

@zFernand0 do you have squash and merge enabled on the repo, it would be nice to keep the history as tidy as possible right? 🫣 Avoiding a load of main merges and revert commits is preferable?

@Joe-Winchester
Copy link
Member

@zFernand0 do you have squash and merge enabled on the repo, it would be nice to keep the history as tidy as possible right? 🫣 Avoiding a load of main merges and revert commits is preferable?

Just checked the settings and it looks like it's enabled

image

Copy link
Contributor

@davenice davenice left a comment

Choose a reason for hiding this comment

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

Excellent. My preference is to organise commits before they're merged, simply because it's too easy for the person merging to click the wrong button and then my stream of commit-consciousness is listed in the history forever. Sadly I've been on both sides of that storyline!

But it sounds like we've got that covered today with the button :-) 👍

@davenice davenice dismissed AndrewTwydell’s stale review December 18, 2024 18:20

changes have been made

@chinmdas
Copy link
Contributor Author

Hi @Joe-Winchester @zFernand0 , Do I have merge access?

Copy link
Contributor

@AndrewTwydell AndrewTwydell left a comment

Choose a reason for hiding this comment

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

LGTM - Thanks @chinmdas!

I don't think we have the powers to merge to main, no - potentially for the best for the time being!

@JillieBeanSim
Copy link

Hey @Joe-Winchester can add admin access to the team members you would like to be able to merge PRs 😸
Should be able to do this under the Settings tab then setup group/users within the Access>Collaborators & Teams

@zFernand0
Copy link
Member

As it stands right now, only these two teams have access to merge PRs to protected branches on this monorepo

As for adding more people to the mainteners/admins group(s), that's always up to the squad (it doesn't matter how new someone might be) 😋

If y'all want to add a few more folks to the CICS Administrators, feel free to bring it up on the next standup 😋


As for squashing vs merging...
I'm personally a fan of squashing PRs, however We may need to so some testing with the deployment workflow before we decide to move to a squash-approach 😋

Thanks Joe for looking into the squash vs merge configuration.

@zFernand0
Copy link
Member

zFernand0 commented Dec 18, 2024

Hey @chinmdas,
Did this commit resolve the problem DCO problem? c01d2b6 🥳
Or did someone manually clicked the Set DCO to Pass button? 🤔

Copy link
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

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

I really like the new flow 🙏

As a follow up PR, we could have a Create CICS profile which just run the zowe.all.config.initwith a warning to make people aware that new profiles will be created in their existing zowe.config.json file 😋

(Similar to the Zowe Explorer flow) 😋

@zFernand0 zFernand0 linked an issue Dec 18, 2024 that may be closed by this pull request
@zFernand0 zFernand0 merged commit d75f17d into main Dec 18, 2024
16 checks passed
@zFernand0 zFernand0 deleted the clean-up-obsolete-web-page branch December 18, 2024 20:31
@zFernand0
Copy link
Member

Labeling: release-minor to include these enhancements from the SDK
image

@zFernand0 zFernand0 added the release-minor Indicates a minor feature has been added label Dec 18, 2024
@davenice
Copy link
Contributor

Thanks Fernando - Chin can elaborate more but this PR was to make the buttons that are there, work. I think Chin was considering a bit more shifting round to look more like Zowe Explorer upcoming (but lower priority than fixing the broken bits!). He might have your warning in mind already 🤔

In terms of maintainer/admin/committer - in our org anybody can merge a PR as long as it has the mandated set of approvals from the right people - so people often merge their own PRs at a point where they're ready, hence our confusion.

Fundamentally we need an approval from you guys so if you have to click the merge button as well then so be it. It's not causing a particular problem. 👍

Copy link

Release succeeded for the main branch. 🎉

The following packages have been published:

Powered by Octorelease 🚀

@chinmdas
Copy link
Contributor Author

Hey @chinmdas, Did this commit resolve the problem DCO problem? c01d2b6 🥳 Or did someone manually clicked the Set DCO to Pass button? 🤔

Hi @zFernand0 ,
That commit did not pass DCO. I manually clicked Set DCO to Pass to check what option that gives me in further steps, but little I knew that one click will set the DCO to pass.

@chinmdas
Copy link
Contributor Author

I really like the new flow 🙏

As a follow up PR, we could have a Create CICS profile which just run the zowe.all.config.initwith a warning to make people aware that new profiles will be created in their existing zowe.config.json file 😋

(Similar to the Zowe Explorer flow) 😋

Sure, We will do this.
Along with that, there were other scenario's that @AndrewTwydell and @davenice spotted which needs improvisation in order to stay in sync with ZE Flow. I will be creating new change set for that.
Thanks for your promptness @zFernand0 ..!!

@zFernand0
Copy link
Member

zFernand0 commented Dec 19, 2024

Hey @chinmdas, Did this commit resolve the problem DCO problem? c01d2b6 🥳 Or did someone manually clicked the Set DCO to Pass button? 🤔

Hi @zFernand0 , That commit did not pass DCO. I manually clicked Set DCO to Pass to check what option that gives me in further steps, but little I knew that one click will set the DCO to pass.

Ideally, we should avoid clicking that button. There is just no configuration to remove it yet. More details:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-minor Indicates a minor feature has been added released
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

Have option to create team config file
7 participants