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

remove random src folder full of json files #1519

Merged
merged 6 commits into from Oct 11, 2021
Merged

remove random src folder full of json files #1519

merged 6 commits into from Oct 11, 2021

Conversation

JillieBeanSim
Copy link
Contributor

we somehow have a random src folder in the next branch that is all json files from the localization that looks like it's from running prettier in this commit 7b96ce2

image

Types of changes

What types of changes does your code introduce to Zowe Explorer?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Updates to Documentation or Tests (if none of the other choices apply)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This checklist will be used as reference for both the contributor and the reviewer

  • I have read the CONTRIBUTOR GUIDANCE wiki
  • PR title follows Conventional Commits Guidelines
  • PR Description is included
  • gif or screenshot is included if visual changes are made
  • yarn workspace vscode-extension-for-zowe vscode:prepublish has been executed
  • All checks have passed (DCO, Jenkins and Code Coverage)
  • I have added unit test and it is passing
  • I have added integration test and it is passing
  • There is coverage for the code that I have added
  • I have tested it manually and there are no regressions found
  • I have added necessary documentation (if appropriate)
  • Any PR dependencies have been merged and published (if appropriate)

Reminder

After a PR is merged into the master branch, create a PR from master to the next branch resolving any conflicts.

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
@JillieBeanSim JillieBeanSim self-assigned this Oct 6, 2021
@JillieBeanSim JillieBeanSim added this to the vNext 0.2 milestone Oct 6, 2021
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
@JillieBeanSim
Copy link
Contributor Author

we may want to check that all 1.18.1 items are brought in as well since we noticed that the @zowe/cli dependency that was added to the ZE API package.json was missing

@lauren-li
Copy link
Contributor

we may want to check that all 1.18.1 items are brought in as well since we noticed that the @zowe/cli dependency that was added to the ZE API package.json was missing

It looks like all the 1.18.1 items are in this PR branch. The only thing I saw that was different was the deployment.yml, which I believe is intentionally different between master and next.

lauren-li
lauren-li previously approved these changes Oct 6, 2021
Copy link
Contributor

@lauren-li lauren-li left a comment

Choose a reason for hiding this comment

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

This PR looks good to me! Thanks @JillieBeanSim for removing the extra folder and adding the Zowe CLI dependency back in to the Zowe Explorer API.

@JillieBeanSim
Copy link
Contributor Author

JillieBeanSim commented Oct 7, 2021

we may want to check that all 1.18.1 items are brought in as well since we noticed that the @zowe/cli dependency that was added to the ZE API package.json was missing

It looks like all the 1.18.1 items are in this PR branch. The only thing I saw that was different was the deployment.yml, which I believe is intentionally different between master and next.

@lauren-li The deployment.yml should be the same, I will take a look at it

UPDATE: I just did a compare of this branch and master branch and no differences found in deployment.yml. We do have difference between the branches on zowe-explorer-ci.yml & zowe-explorer-ftp-ci.yml but that is only the node versions since node 10 isn't supported on the next branch.

@@ -37,8 +37,7 @@
"promptCredentials.updateConfigCreds.infoMessage": "Credentials for future use with profile {0} will need to be updated in the Zowe config file or by using the command 'zowe config secure'.",
"promptCredentials.saveCredentials.button": "Save Credentials",
"promptCredentials.doNotSave.button": "Do Not Save",
"promptCredentials.saveCredentials.infoMessage": "Save entered credentials for future use with profile: {0}?\nSaving credentials will update the local yaml file.",
"promptCredentials.saveCredentials.cancelled": "Operation Cancelled",
"promptCredentials.saveCredentials.infoMessage": "Save entered credentials for future use with profile: {0}? Saving credentials will update the local yaml file.",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure why this changed either, will look at that too

Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
@JillieBeanSim
Copy link
Contributor Author

JillieBeanSim commented Oct 7, 2021

how do we fix an audit with a critical severity of Lodash if no patch? GHSA-8p5q-j9m2-g8wr

Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
@JillieBeanSim
Copy link
Contributor Author

JillieBeanSim commented Oct 7, 2021

resolving ssh2 npm audit seems to break everything, removing that resolution until we figure this out. This is a dependency of @zowe/cli, do they have a version with the fix that we can update to?

image

@lauren-li
Copy link
Contributor

we may want to check that all 1.18.1 items are brought in as well since we noticed that the @zowe/cli dependency that was added to the ZE API package.json was missing

It looks like all the 1.18.1 items are in this PR branch. The only thing I saw that was different was the deployment.yml, which I believe is intentionally different between master and next.

@lauren-li The deployment.yml should be the same, I will take a look at it

UPDATE: I just did a compare of this branch and master branch and no differences found in deployment.yml. We do have difference between the branches on zowe-explorer-ci.yml & zowe-explorer-ftp-ci.yml but that is only the node versions since node 10 isn't supported on the next branch.

@JillieBeanSim Apologies, I double-checked, and I had been comparing the deployment.yml changes made in PR #1442 for the 1.18.1 release against next when I wrote my earlier comment. Thanks for the correction!

@t1m0thyj
Copy link
Member

t1m0thyj commented Oct 7, 2021

resolving ssh2 npm audit seems to break everything, removing that resolution until we figure this out. This is a dependency of @zowe/cli, do they have a version with the fix that we can update to?

image

@zowe/cli@zowe-v1-lts has been updated to fix the ssh2 vulnerability. You may still encounter this error coming from the "cpu-features" package. It is an optional dependency making use of native binaries that was added to recent versions of ssh2.

We have decided to exclude "cpu-features" from the CLI TGZ on zowe.org because it must be built on each user's system at install time. Perhaps you'll also want to exclude "cpu-features" in your webpack config.

@lauren-li
Copy link
Contributor

resolving ssh2 npm audit seems to break everything, removing that resolution until we figure this out. This is a dependency of @zowe/cli, do they have a version with the fix that we can update to?

I have created PR #1521, which updates the Zowe CLI version to 7.0.0-next.202109281609. As the ssh2 audit issues were resolved in Zowe CLI version 7.0.0-next.202109141650, PR #1521 should take care of this for Zowe Explorer's next branch. (Thanks @JillieBeanSim for your research on this!)

Copy link
Contributor

@lauren-li lauren-li left a comment

Choose a reason for hiding this comment

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

This looks good to me! I do not see any regressions as a result of this PR. Per @crawr and @VitGottwald's update, the lodash audit issue might be revoked in the near future (and Zowe Explorer does not use the template function of lodash, so we are not affected anyway).

The ssh2 audit issue can be handled in a separate PR (such as #1521) before the next vNext release.

Thanks @JillieBeanSim for cleaning up the folders (and taking care of some dependency issues)!

@@ -894,7 +894,7 @@ export class Profiles extends ProfilesCache {
const doNotSaveButton = localize("promptCredentials.doNotSave.button", "Do Not Save");
const infoMsg = localize(
"promptCredentials.saveCredentials.infoMessage",
"Save entered credentials for future use with profile: {0}? Saving credentials will update the local yaml file.",
"Save entered credentials for future use with profile: {0}?\nSaving credentials will update the local yaml file.",
Copy link
Contributor

Choose a reason for hiding this comment

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

The \n here doesn't seem to create a newline for me in the pop-up message. I do see that this is in master branch, so I think it's okay to go into next, but I was curious if others can see its effect.

Copy link
Contributor

@crawr crawr 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 for cleaning this up in preparation for the release

@crawr crawr merged commit 5252a48 into next Oct 11, 2021
@JillieBeanSim JillieBeanSim deleted the next-fix branch October 22, 2021 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants