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

credentials manager initial poc #24

Merged
merged 5 commits into from
May 19, 2022

Conversation

johnnyon-amzn
Copy link
Contributor

###Description
Allows users to create multiple login profiles and access keys.
Added a pop up window for handling credentials and saving new profiles.
Created an edit button that also updates config.
Changed config.json file to be able to store credentials.

###Issues resolved
#8

@johnnyon-amzn johnnyon-amzn requested a review from a team May 16, 2022 20:45
@johnnyon-amzn johnnyon-amzn self-assigned this May 16, 2022
Copy link
Member

@zhongnansu zhongnansu left a comment

Choose a reason for hiding this comment

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

Please add license headers for all code files. You may consider some utility tools in VScode that can automatically add header for all files in the project.

@@ -0,0 +1,26 @@
const path = require('path');
const fs = require("fs");
const CONFIG_PATH = path.join(__dirname, "config.json");
Copy link
Member

Choose a reason for hiding this comment

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

What's this config file? Any reference in the codebase, I did a simple search, couldn't find anything.

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 how to approach. By default it should be a file in src that is an empty object. However, when testing and using the app, it gets populated with access key information which I don't want to push

Copy link
Member

Choose a reason for hiding this comment

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

OK so this file will be generated by code? I am good with that. Any reference in README or comments in code files will be helpful

src/config.js Outdated
function getConfig(name, configPath=CONFIG_PATH) {
let config = fs.readFileSync(configPath);
config = JSON.parse(config.toString());
console.log('config', config);
Copy link
Member

Choose a reason for hiding this comment

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

why print in console?

Copy link
Member

Choose a reason for hiding this comment

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

will this expose user data?

},
getConfig: (name) => configLibrary.getConfig(name),
setConfig: (config, callback) => configLibrary.setConfig(config, callback),

Copy link
Member

Choose a reason for hiding this comment

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

remove empty line

src/renderer.js Outdated
function getActiveConfig() {
let profile = document.getElementById("profile");
var name = profile.options[profile.selectedIndex].value;
console.log('name', name);
Copy link
Member

Choose a reason for hiding this comment

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

could you check similar console.log, and see if they are really necessary to remove. If it's for dev purpose, you can add some TODO comments, so people who work on this later will know and remove.

src/renderer.js Outdated
input.setAttribute("value", config[map.key])
}
config = window.electron.getConfig();
let profile = document.getElementById("profile");
Copy link
Member

Choose a reason for hiding this comment

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

Np: you may want to make id as constants for reuse. See another usage at line 27

Signed-off-by: Johnny On <[email protected]>
Signed-off-by: Johnny On <[email protected]>
Signed-off-by: Johnny On <[email protected]>
Signed-off-by: Johnny On <[email protected]>
@seraphjiang seraphjiang requested a review from a team May 18, 2022 18:20
@johnnyon-amzn johnnyon-amzn merged commit 90a75e0 into opensearch-project:dev May 19, 2022
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.

2 participants