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

Gitops GitHub create wizard #36

Merged
merged 23 commits into from
Nov 29, 2024

Conversation

Abiji-2020
Copy link
Contributor

Created the complete GitOps flow for GitHub

  • Created component based design
  • Added unit tests

closes #19
@quest-bot loot #19

Test Coverage

image

Demo video

Permit-gitops.flow.test.mp4

@Abiji-2020
Copy link
Contributor Author

@quest-bot loot #19

@quest-bot quest-bot bot added the ⚔️ Quest Tracks quest-bot quests label Nov 23, 2024
Copy link

quest-bot bot commented Nov 23, 2024

Quest PR submitted! image Quest PR submitted!

@Abiji-2020, you are attempting to solve the issue and loot this Quest. Will you be successful?


Questions? Check out the docs.

@quest-bot quest-bot bot mentioned this pull request Nov 23, 2024
@Abiji-2020
Copy link
Contributor Author

Abiji-2020 commented Nov 23, 2024

@gemanor @orweis can you review this PR?

@gemanor
Copy link
Collaborator

gemanor commented Nov 24, 2024

@Abiji-2020, it looks good at first sight; thanks for your contribution.

To continue with a more detailed review, I will ask you to proceed with the following:

  1. Merge with the main repo (opened PR) so we can ensure lint
  2. Take the API key either from the keychain and add an option to provide it as an argument named --key. You can see an example in the check commands

Please let me know when you're done, and I'll proceed with more detailed review.

@orweis
Copy link
Collaborator

orweis commented Nov 24, 2024

The copy SSH key stage is very clunky -

  • You need to edit out the newlines
  • It's unclear which part to copy paste (Even just visually)

@Abiji-2020
Copy link
Contributor Author

@Abiji-2020, it looks good at first sight; thanks for your contribution.

To continue with a more detailed review, I will ask you to proceed with the following:

  1. Merge with the main repo (opened PR) so we can ensure lint
  2. Take the API key either from the keychain and add an option to provide it as an argument named --key. You can see an example in the check commands

Please let me know when you're done, and I'll proceed with more detailed review.

@gemanor I have modified, to use the API Key either from the command line as option. and if it is not given, we will take it the default API value stored in keychain.

example

image

@Abiji-2020
Copy link
Contributor Author

The copy SSH key stage is very clunky -

  • You need to edit out the newlines

  • It's unclear which part to copy paste (Even just visually)

image

I have edited out the newlines and copying to clipboard is necessary? If wanted let me add it too

@gemanor
Copy link
Collaborator

gemanor commented Nov 24, 2024

@Abiji-2020, it looks good at first sight; thanks for your contribution.
To continue with a more detailed review, I will ask you to proceed with the following:

  1. Merge with the main repo (opened PR) so we can ensure lint
  2. Take the API key either from the keychain and add an option to provide it as an argument named --key. You can see an example in the check commands

Please let me know when you're done, and I'll proceed with more detailed review.

@gemanor I have modified, to use the API Key either from the command line as option. and if it is not given, we will take it the default API value stored in keychain.

example

image

It should use the --key arguments only if they are present and use the keychain as the default, not as a default argument to the command. Please look at the check commands and use the same convention as they are there.

@gemanor
Copy link
Collaborator

gemanor commented Nov 24, 2024

Also, @Abiji-2020, I'm trying to run the tests locally, but couldn't. Please merge from Main, fix all the lint errors, and ensure the test is passing.

@gemanor
Copy link
Collaborator

gemanor commented Nov 24, 2024

Another issue is the follwoing error that I get when running it:

node:internal/modules/cjs/loader:1225
  const err = new Error(message);
              ^

Error: Cannot find module '../build/Release/keytar.node'
Require stack:
- /Users/gemanor/dev/cli-gitops/node_modules/keytar/lib/keytar.js
    at Module._resolveFilename (node:internal/modules/cjs/loader:1225:15)
    at Module._load (node:internal/modules/cjs/loader:1051:27)
    at Module.require (node:internal/modules/cjs/loader:1311:19)
    at require (node:internal/modules/helpers:179:18)
    at Object.<anonymous> (/Users/gemanor/dev/cli-gitops/node_modules/keytar/lib/keytar.js:1:14)
    at Module._compile (node:internal/modules/cjs/loader:1469:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1548:10)
    at Module.load (node:internal/modules/cjs/loader:1288:32)
    at Module._load (node:internal/modules/cjs/loader:1104:12)
    at cjsLoader (node:internal/modules/esm/translators:346:17) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [ '/Users/gemanor/dev/cli-gitops/node_modules/keytar/lib/keytar.js' ]
}

@Abiji-2020
Copy link
Contributor Author

Abiji-2020 commented Nov 24, 2024

Another issue is the follwoing error that I get when running it:

node:internal/modules/cjs/loader:1225
  const err = new Error(message);
              ^

Error: Cannot find module '../build/Release/keytar.node'
Require stack:
- /Users/gemanor/dev/cli-gitops/node_modules/keytar/lib/keytar.js
    at Module._resolveFilename (node:internal/modules/cjs/loader:1225:15)
    at Module._load (node:internal/modules/cjs/loader:1051:27)
    at Module.require (node:internal/modules/cjs/loader:1311:19)
    at require (node:internal/modules/helpers:179:18)
    at Object.<anonymous> (/Users/gemanor/dev/cli-gitops/node_modules/keytar/lib/keytar.js:1:14)
    at Module._compile (node:internal/modules/cjs/loader:1469:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1548:10)
    at Module.load (node:internal/modules/cjs/loader:1288:32)
    at Module._load (node:internal/modules/cjs/loader:1104:12)
    at cjsLoader (node:internal/modules/esm/translators:346:17) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [ '/Users/gemanor/dev/cli-gitops/node_modules/keytar/lib/keytar.js' ]
}

This issue is that keytar is missing right?

UPDATE: I cannot reproduce this issue, could give me the steps to reproduce?

@Abiji-2020
Copy link
Contributor Author

Abiji-2020 commented Nov 24, 2024

@gemanor apologies, as I haven't added the test to the scripts which are added now, and in the linting it shows warnings of the empty dependencies in useEffect, as I wanted to call them only once, I have used an empty array (to call them when only mounted). and the option --key was added as per the conditions.

If we use the argument --key then it will go with the apiKey in --key argument else it will go with the default key in keychain.

There are some existing errors in https://github.com/permitio/permit-cli/blob/main/test.tsx

@gemanor
Copy link
Collaborator

gemanor commented Nov 24, 2024

Another issue is the follwoing error that I get when running it:

node:internal/modules/cjs/loader:1225
  const err = new Error(message);
              ^

Error: Cannot find module '../build/Release/keytar.node'
Require stack:
- /Users/gemanor/dev/cli-gitops/node_modules/keytar/lib/keytar.js
    at Module._resolveFilename (node:internal/modules/cjs/loader:1225:15)
    at Module._load (node:internal/modules/cjs/loader:1051:27)
    at Module.require (node:internal/modules/cjs/loader:1311:19)
    at require (node:internal/modules/helpers:179:18)
    at Object.<anonymous> (/Users/gemanor/dev/cli-gitops/node_modules/keytar/lib/keytar.js:1:14)
    at Module._compile (node:internal/modules/cjs/loader:1469:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1548:10)
    at Module.load (node:internal/modules/cjs/loader:1288:32)
    at Module._load (node:internal/modules/cjs/loader:1104:12)
    at cjsLoader (node:internal/modules/esm/translators:346:17) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [ '/Users/gemanor/dev/cli-gitops/node_modules/keytar/lib/keytar.js' ]
}

This issue is that keytar is missing right?

UPDATE: I cannot reproduce this issue, could give me the steps to reproduce?

What version of Node.js are you using?

@gemanor
Copy link
Collaborator

gemanor commented Nov 24, 2024

Works for me now. Will do CR soon.

Can you tell me what do you mean by policy_name? where is that being used?

@Abiji-2020 Abiji-2020 requested a review from gemanor November 25, 2024 12:50
@Abiji-2020
Copy link
Contributor Author

Abiji-2020 commented Nov 25, 2024

@gemanor Apart from the AuthProvider Everything is modified. I am waiting to know more about how to implement the AuthProvider.

export default function Run({ options: { opa } }: Props) {
return (
<AuthProvider>
<PDPCommand opa={opa} />
</AuthProvider>
);

In this we are making AuthProvider as the parent node and the components as the child node, so should we encapsulate all the components within the AuthProvider or we can do it as in the place before the project state.

{state === 'project' && (
<SelectProject

{	state === 'apiKey' && (<AuthProvider>
	{	const {authToken} = useAuth();
		setApiKey(authToken);
		setState('project');
	}
	</AuthProvider>)
}

@gemanor
Copy link
Collaborator

gemanor commented Nov 25, 2024

We don't have now "parent" component, so you need to wrap the GitHub create command, and encapsulate its functionality that needs the API key to components.

@Abiji-2020
Copy link
Contributor Author

Abiji-2020 commented Nov 26, 2024

We don't have now "parent" component, so you need to wrap the GitHub create command, and encapsulate its functionality that needs the API key to components.

I am unable to grasp the things, could you elaborate more @gemanor , and instead of auth provider can't we use

};
export const loadAuthToken = async (): Promise<string> => {
const token = await getPassword(
KEYSTORE_PERMIT_SERVICE_NAME,
DEFAULT_PERMIT_KEYSTORE_ACCOUNT,
);
if (!token) {
throw new Error(
'No token found, use `permit login` command to get an auth token',
);
}
return token;
};

which simplifies and also gets the APIkey from Keychain.

Since AuthProvider also provides the same way

const fetchAuthToken = async () => {
try {
const token = await loadAuthToken();
setAuthToken(token);
} catch (error) {
setError(error instanceof Error ? error.message : String(error));
}
setLoading(false);

@gemanor
Copy link
Collaborator

gemanor commented Nov 26, 2024

The intent to use the AuthProvider, is to make its logic shared across components, so we can later extend its functionality with other stuff. The load is a util function, not logical function.
This is why it is important to use the AuthProvider.

@Abiji-2020
Copy link
Contributor Author

So shall I create a new file with the complete github component and have an encapultate in the command similar to how we have exactly in https://github.com/permitio/permit-cli/blob/main/source/commands/pdp/run.tsx ? @gemanor

@Abiji-2020
Copy link
Contributor Author

@gemanor I have added the AuthProvider by creating a new component similar to how it is done in https://github.com/permitio/permit-cli/blob/main/source/commands/pdp/run.tsx and also there are no linting errors.

Copy link
Collaborator

@gemanor gemanor left a comment

Choose a reason for hiding this comment

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

I tested the flow and it's not working. Please fix that (or report errors if there are any).

Also, left some other comments. Please address them.

source/commands/gitops/create/github.tsx Outdated Show resolved Hide resolved
source/lib/gitops/utils.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
source/components/gitops/RepositoryKey.tsx Outdated Show resolved Hide resolved
source/components/gitops/RepositoryKey.tsx Outdated Show resolved Hide resolved
source/components/gitops/SSHKey.tsx Outdated Show resolved Hide resolved
source/components/gitops/SSHKey.tsx Outdated Show resolved Hide resolved
source/lib/gitops/utils.ts Show resolved Hide resolved
source/components/gitops/SSHKey.tsx Show resolved Hide resolved
source/components/gitops/SSHKey.tsx Show resolved Hide resolved
@Abiji-2020
Copy link
Contributor Author

I tested the flow and it's not working. Please fix that (or report errors if there are any).

Also, left some other comments. Please address them.

yeah it is broken in the BranchName component, it is fixed.

@Abiji-2020 Abiji-2020 requested a review from gemanor November 27, 2024 13:05
@gemanor
Copy link
Collaborator

gemanor commented Nov 29, 2024

Now I get an error when trying to save:

GitOps Configuration Wizard - GitHub


 Error: Invalid Configuration [object Object]

@Abiji-2020
Copy link
Contributor Author

Now I get an error when trying to save:

GitOps Configuration Wizard - GitHub


 Error: Invalid Configuration [object Object]

@gemanor again i tested my workflow it seems just to be working fine. could you give me more info on this?

@gemanor
Copy link
Collaborator

gemanor commented Nov 29, 2024

Okay, the problem is the browser login flow. It works only with key, let me try to fix that.

Another problem, is handling error with activate call. Even when it's succeed, it gets error after saving.
Can you take a look on that please?

@Abiji-2020
Copy link
Contributor Author

@gemanor I think we need to delay the activate component a little bit as, when i try to activate it immediately it showed me error but for the same when i paused for a minute and then try to activate it, it worked.

@gemanor
Copy link
Collaborator

gemanor commented Nov 29, 2024

The logic should be a little different. The policy repository is on status of pending and you can't activate a pending repository.

It is not about delaying in 1 minute; it is about waiting for the repository status to be valid.

We can solve it with two options, the first is the easiest IMO:

  1. Remove the activate step, and instead give the user an optional --inactive argument on the command. When you're creating the repository (configure step), you'll do the activate_when_validated false only if the --inactive is present. By default, you'll pass it as true.
  2. Call the get repository in 10 seconds interval until it's validated.

Again, IMO, removing the activate step and instead use argument to disable the activate_when_validated attribute when creating, is the best option

package.json Outdated Show resolved Hide resolved
@Abiji-2020
Copy link
Contributor Author

The logic should be a little different. The policy repository is on status of pending and you can't activate a pending repository.

It is not about delaying in 1 minute; it is about waiting for the repository status to be valid.

We can solve it with two options, the first is the easiest IMO:

  1. Remove the activate step, and instead give the user an optional --inactive argument on the command. When you're creating the repository (configure step), you'll do the activate_when_validated false only if the --inactive is present. By default, you'll pass it as true.
  2. Call the get repository in 10 seconds interval until it's validated.

Again, IMO, removing the activate step and instead use argument to disable the activate_when_validated attribute when creating, is the best option

@gemanor I have modified the code to go with the option1 flow

Copy link
Collaborator

@gemanor gemanor left a comment

Choose a reason for hiding this comment

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

Nice work!

@gemanor gemanor merged commit 51dea81 into permitio:main Nov 29, 2024
3 checks passed
@Abiji-2020
Copy link
Contributor Author

@gemanor could you check on the quest loot is not yet released for this ?

Copy link

quest-bot bot commented Dec 9, 2024

🧚 @Abiji-2020 congratulations for completing Quest #19

💰 A reward of $300 has been credited to you.

To claim your $300 reward follow the instructions here.

Questions? Check out the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚔️ Quest Tracks quest-bot quests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Permit GitOps Flow Wizard
3 participants