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

Added support for ltpa token #219

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft

Added support for ltpa token #219

wants to merge 12 commits into from

Conversation

enamkhan
Copy link
Contributor

@enamkhan enamkhan commented Jan 29, 2025

This pr is a work in progress to add support for a using the ltpa token.

#217

How to Test

Review Checklist
I certify that I have:

Additional Comments

@enamkhan
Copy link
Contributor Author

@davenice I'm just going to refactor now and remove the profile config.

@codecov-commenter
Copy link

codecov-commenter commented Jan 29, 2025

Codecov Report

Attention: Patch coverage is 13.33333% with 78 lines in your changes missing coverage. Please review.

Project coverage is 31.29%. Comparing base (1087db6) to head (50424cb).
Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
packages/vsce/src/trees/CICSTree.ts 13.95% 37 Missing ⚠️
packages/vsce/src/utils/profileUtils.ts 26.31% 14 Missing ⚠️
packages/vsce/src/trees/CICSRegionsContainer.ts 0.00% 4 Missing ⚠️
...trees/CICSCombinedTrees/CICSCombinedLibraryTree.ts 0.00% 2 Missing ⚠️
...ees/CICSCombinedTrees/CICSCombinedLocalFileTree.ts 0.00% 2 Missing ⚠️
...rees/CICSCombinedTrees/CICSCombinedPipelineTree.ts 0.00% 2 Missing ⚠️
...trees/CICSCombinedTrees/CICSCombinedProgramTree.ts 0.00% 2 Missing ⚠️
.../CICSCombinedTrees/CICSCombinedTCPIPServiceTree.ts 0.00% 2 Missing ⚠️
...rc/trees/CICSCombinedTrees/CICSCombinedTaskTree.ts 0.00% 2 Missing ⚠️
...s/CICSCombinedTrees/CICSCombinedTransactionTree.ts 0.00% 2 Missing ⚠️
... and 5 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #219      +/-   ##
==========================================
+ Coverage   28.56%   31.29%   +2.72%     
==========================================
  Files         147      147              
  Lines        5031     5059      +28     
  Branches      882      884       +2     
==========================================
+ Hits         1437     1583     +146     
+ Misses       3594     3476     -118     

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

Signed-off-by: enam-khan <[email protected]>
Signed-off-by: enam-khan <[email protected]>
Signed-off-by: enam-khan <[email protected]>
Signed-off-by: enam-khan <[email protected]>
@enamkhan enamkhan force-pushed the khane-use-ltpa-token branch from f88f1d5 to fd79504 Compare February 3, 2025 15:24
Signed-off-by: enam-khan <[email protected]>
Signed-off-by: enam-khan <[email protected]>
@enamkhan enamkhan force-pushed the khane-use-ltpa-token branch from 7c4bd64 to 1cd8078 Compare February 4, 2025 13:22
Signed-off-by: enam-khan <[email protected]>
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.

In a lot of the resource trees (CICSProgramTree), we're not using the new getters to get the session - might be nice if possible?

Might be nice to be able to do this.getSession() here like the combination trees.

const programResponse = await getResource(this.parentRegion.parentSession.session, {

@@ -282,9 +268,8 @@ export class ProfileManagement {
/**
* Return all the regions in a given plex
*/
public static async getRegionInfoInPlex(plex: CICSPlexTree): Promise<any[]> {
public static async getRegionInfoInPlex(plex: CICSPlexTree, session: Session): Promise<any[]> {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can probably get the session (2nd arg) from the plex tree (1st arg) using getParent or getSession, or a combination, right? Although it's not exactly messy currently so optional.

@@ -204,4 +209,8 @@ export class CICSCombinedWebServiceTree extends TreeItem {
public getParent() {
return this.parentPlex;
}

public getSession() {
return this.getParent().getParent().getSession();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this just be this.getParent().getSession() as the parent's getSession will also keep going up the tree??

Copy link
Contributor

Choose a reason for hiding this comment

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

If so, same for all combination trees

@JTonda JTonda requested a review from zFernand0 February 6, 2025 16:05
Signed-off-by: enam-khan <[email protected]>
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.

LGTM! 😋

Haven't had a chance to test this, but the changes make sense to me! 😋

Copy link
Member

Choose a reason for hiding this comment

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

It feels like we have to make the same/similar changes across all/most CICSCombinedTrees.
Wondering if there is some consolidation that can be done (in a future PR) to create a BaseCombinedTree class that the others extend 😋

Copy link
Contributor

Choose a reason for hiding this comment

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

This refactor is actually in progress in this PR. Because all the resource and combined resource trees are practically identical, we can use common tree classes for them all :D

this.session = new imperative.Session({
type: "basic",
type: SessConstants.AUTH_TYPE_TOKEN,
storeCookie: true,
Copy link
Member

Choose a reason for hiding this comment

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

I know I mentioned that perhaps we could move some of the LTPA token support to the CLI.
However, it makes sense the way this is being done here since (as you mentioned on the last standup) this is all handled by Imperative.

Curious if the CICs plug-in could make use the the LTPA token when the CLI is in Daemon mode. 😋
No need to worry about testing this, since people haven't asked for this feature yet 😋

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants