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.

Sign up for GitHub

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

Jump to bottom

chore: CLI Profile Remove - Check and Use config file before checking system keychain. #126

Merged
merged 2 commits into from
Jun 23, 2021
Merged

chore: CLI Profile Remove - Check and Use config file before checking system keychain. #126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5ea3ded
chore: CLI Profile Remove - Check and Use config file before checking…
ravali-rimmalapudi Jun 22, 2021
6ce23f4
Remves the check removeProfile while updating the project.
ravali-rimmalapudi Jun 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions src/services/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,9 +129,13 @@ class ConfigData {
}

removeProfile(profileToRemove) {
this.projects = this.projects.filter((profile) => {
Copy link
Contributor Author

@ravali-rimmalapudi ravali-rimmalapudi Jun 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Regarding L154, what is the better way to remove the keys form keytar?

  1. Do we need to directly call secureStorage method at L154?
  2. Do we need to let user know that we are deleting the key in keytar and updating it in config file or just log the status of key deletion from keytar?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the logic for dealing with secureStorage should be moved here so it can be easily updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But when I am trying to use secureStorage in this file, it's leading to circular dependency.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't they be removed as part of deleteLocalKey already even before removeProfile is called?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it will be removed in case of profile:remove cmd, but there is another case where we are updating the existing profile, if the profile found in projects we are removing it from projects (but API keys we are not removing from keytar) and updating in profiles key. This flow will come in case of profile:create existingProfileName.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, we have a separate ticket for the update case. Perhaps we could call remove and create there for existing profiles, but let's not put that mix that with this

return profile.id !== profileToRemove.id;
});
if (this.profiles[profileToRemove.id]) {
delete this.profiles[profileToRemove.id];
} else {
this.projects = this.projects.filter((profile) => {
return profile.id !== profileToRemove.id;
});
}
if (profileToRemove.id === this.activeProfile) {
this.activeProfile = null;
}
Expand Down
129 changes: 99 additions & 30 deletions test/services/config.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,46 +210,115 @@ describe('services', () => {
describe('ConfigData.removeProfile', () => {
test.it('remove a profile that does not exist', () => {
const configData = new ConfigData();
configData.addProfile('firstProfile', constants.FAKE_ACCOUNT_SID);
configData.addProfile('secondProfile', 'new_account_SID');
configData.addProfile('thirdProfile', 'newest_account_SID');
configData.addProject('firstProfile', constants.FAKE_ACCOUNT_SID);
configData.addProfile(
'secondProfile',
'new_account_SID',
'',
constants.FAKE_API_KEY,
constants.FAKE_API_SECRET,
);
configData.addProfile(
'thirdProfile',
'newest_account_SID',
'',
constants.FAKE_API_KEY,
constants.FAKE_API_SECRET,
);
const fakeProfile = {
id: 'DOES_NOT_EXIST',
accountSid: 'fake_SID',
};
const originalLength = configData.projects.length;
const originalProfilesLength = Object.keys(configData.profiles).length;
configData.removeProfile(fakeProfile);

expect(configData.projects.length).to.equal(originalLength);
expect(Object.keys(configData.profiles).length).to.equal(originalProfilesLength);
});

test.it('removes profile from projects', () => {
const configData = new ConfigData();
configData.addProject('firstProfile', constants.FAKE_ACCOUNT_SID);
configData.addProject('secondProfile', 'new_account_SID');
configData.addProject('thirdProfile', 'newest_account_SID');
configData.addProfile(
'fourthProfile',
'fourth_account_SID',
'',
constants.FAKE_API_KEY,
constants.FAKE_API_SECRET,
);
const profile = configData.getProfileById('secondProfile');
const originalLengthProfiles = Object.keys(configData.profiles).length;
configData.removeProfile(profile);

expect(configData.projects[1].id).to.equal('thirdProfile');
expect(configData.projects[1].accountSid).to.equal('newest_account_SID');
expect(Object.keys(configData.profiles).length).to.equal(originalLengthProfiles);
});
test.it('removes profile from profiles', () => {
const configData = new ConfigData();
configData.addProject('firstProfile', constants.FAKE_ACCOUNT_SID);
configData.addProject('secondProfile', 'new_account_SID');
configData.addProfile('thirdProfile', 'newest_account_SID', '', 'third_api_key', 'third_api_secret');
configData.addProfile(
'fourthProfile',
'fourth_account_SID',
'',
constants.FAKE_API_KEY,
constants.FAKE_API_SECRET,
);
const profile = configData.getProfileById('thirdProfile');
const originalLengthProjects = configData.projects.length;
configData.removeProfile(profile);

expect(configData.projects.length).to.equal(originalLengthProjects);
expect(configData.profiles[profile.id]).to.be.undefined;
});

/*
* TODO: To be fixed with profiles:remove functionality
* test.it('removes profile', () => {
* const configData = new ConfigData();
* configData.addProfile('firstProfile', constants.FAKE_ACCOUNT_SID);
* configData.addProfile('secondProfile', 'new_account_SID');
* configData.addProfile('thirdProfile', 'newest_account_SID');
* const profile = configData.getProfileById('secondProfile');
* configData.removeProfile(profile);
*
* expect(configData.projects[1].id).to.equal('thirdProfile');
* expect(configData.projects[1].accountSid).to.equal('newest_account_SID');
* });
*
* test.it('removes active profile', () => {
* const configData = new ConfigData();
* configData.addProfile('firstProfile', constants.FAKE_ACCOUNT_SID);
* configData.addProfile('secondProfile', 'new_account_SID');
* configData.addProfile('thirdProfile', 'newest_account_SID');
* const profile = configData.setActiveProfile('firstProfile');
* configData.removeProfile(profile);
*
* expect(configData.projects[1].id).to.equal('thirdProfile');
* expect(configData.projects[1].accountSid).to.equal('newest_account_SID');
* expect(configData.activeProfile).to.equal(null);
* });
*/
test.it('removes active profile of projects', () => {
const configData = new ConfigData();
configData.addProject('firstProfile', constants.FAKE_ACCOUNT_SID);
configData.addProject('secondProfile', 'new_account_SID');
configData.addProject('thirdProfile', 'newest_account_SID');
configData.addProfile(
'fourthProfile',
'fourth_account_SID',
'',
constants.FAKE_API_KEY,
constants.FAKE_API_SECRET,
);
const profile = configData.setActiveProfile('firstProfile');
const originalLengthProfiles = Object.keys(configData.profiles).length;
configData.removeProfile(profile);

expect(configData.projects[1].id).to.equal('thirdProfile');
expect(configData.projects[1].accountSid).to.equal('newest_account_SID');
expect(configData.activeProfile).to.equal(null);
expect(Object.keys(configData.profiles).length).to.equal(originalLengthProfiles);
});

test.it('removes active profile of profiles', () => {
const configData = new ConfigData();
configData.addProject('firstProfile', constants.FAKE_ACCOUNT_SID);
configData.addProject('secondProfile', 'new_account_SID');
configData.addProfile('thirdProfile', 'newest_account_SID', '', 'third_api_key', 'third_api_secret');
configData.addProfile(
'fourthProfile',
'fourth_account_SID',
'',
constants.FAKE_API_KEY,
constants.FAKE_API_SECRET,
);
const profile = configData.setActiveProfile('thirdProfile');
const originalLengthProjects = configData.projects.length;
configData.removeProfile(profile);

expect(configData.projects.length).to.equal(originalLengthProjects);
expect(configData.profiles[profile.id]).to.be.undefined;
expect(configData.activeProfile).to.equal(null);
});
});
describe('ConfigData.prompts', () => {
test.it('should store prompt acks', () => {
Expand Down