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

Digital Twins page preview - trigger pipeline from client #891

Conversation

VanessaScherma
Copy link
Contributor

Resolves issue #794. It includes a new link in ‘Workbench’ that gives access to the preview of the Digital Twins page. At the moment, only the ‘Execute’ tab has been developed.

.replace(/-/g, ' ')
.replace(/^./, (char) => char.toUpperCase());

const ExecuteDigitalTwin: React.FC<{ name: string }> = (props) => {
Copy link

Choose a reason for hiding this comment

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

Function ExecuteDigitalTwin has 177 lines of code (exceeds 25 allowed). Consider refactoring.

.replace(/-/g, ' ')
.replace(/^./, (char) => char.toUpperCase());

const ExecuteDigitalTwin: React.FC<{ name: string }> = (props) => {
Copy link

Choose a reason for hiding this comment

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

Function ExecuteDigitalTwin has a Cognitive Complexity of 46 (exceeds 5 allowed). Consider refactoring.

import tabs from './DigitalTwinTabData';
import ExecuteTab from '../../components/tab/ExecuteTab';

function DTContent() {
Copy link

Choose a reason for hiding this comment

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

Function DTContent has 38 lines of code (exceeds 25 allowed). Consider refactoring.

Copy link
Contributor

@prasadtalasila prasadtalasila left a comment

Choose a reason for hiding this comment

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

Partial code review done. Please remember to run yarn format and yarn syntax before committing your changes.

client/package.json Show resolved Hide resolved
client/src/components/DigitalTwinCard.tsx Outdated Show resolved Hide resolved
client/src/components/ExecuteDigitalTwin.tsx Outdated Show resolved Hide resolved
client/src/components/tab/ExecuteTab.tsx Outdated Show resolved Hide resolved
client/DEVELOPER.md Show resolved Hide resolved
client/src/route/digitaltwins/DigitalTwinsPreview.tsx Outdated Show resolved Hide resolved
client/src/components/DigitalTwinCard.tsx Outdated Show resolved Hide resolved
@prasadtalasila
Copy link
Contributor

@VanessaScherma Thanks for the PR. Please check the suggestions.

Copy link
Contributor

@prasadtalasila prasadtalasila left a comment

Choose a reason for hiding this comment

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

@VanessaScherma This round of comments completes the code review.

client/src/components/ExecuteDigitalTwin.tsx Outdated Show resolved Hide resolved

const ExecuteDigitalTwin: React.FC<{ name: string }> = (props) => {
const [gitlabInstance, setGitlabInstance] = useState<GitlabInstance | null>(null);
const [digitalTwin, setDigitalTwin] = useState<DigitalTwin | null>(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please simplify these react component properties using react redux store. There are some good examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should redux store only be used with gitlabInstance and digitalTwin properties?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do not understand the question. Can you please elaborate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, the use of redux store does not seem to be usable, as the properties of this component are not global, but concern only one DT. Using redux store would result in continuous overwriting.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a clean solution to this long list of component state?

client/src/util/gitlab.ts Outdated Show resolved Hide resolved
client/src/route/digitaltwins/DigitalTwinsPreview.tsx Outdated Show resolved Hide resolved
client/src/components/ExecuteDigitalTwin.tsx Outdated Show resolved Hide resolved
@prasadtalasila
Copy link
Contributor

@VanessaScherma
Please rebase to feature/distributed-demo branch.

@VanessaScherma
Copy link
Contributor Author

@prasadtalasila
Have you created a pipeline trigger token in the project settings (CI/CD)?
(I actually have to add it to the necessary instructions)

Copy link
Contributor

Choose a reason for hiding this comment

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

please add only the asset components needed for your PR. For example, AddButton component is not required for now.

}

function AssetCardExecute({ asset }: AssetCardExecuteProps) {
const [digitalTwin, setDigitalTwin] = useState<DigitalTwin>(
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably does not belong to component state. Please rethink about this design choice.

@VanessaScherma
Copy link
Contributor Author

@prasadtalasila
I am having some problems with rebasing gitlab to distributed-demo, can you help me please?

I used the command git rebase origin/feature/distributed-demo within the gitlab branch. However, after manually merging the conflicting files, it merged the gitlab branch into the gitlab branch, while distributed-demo is not modified in any way. Could you please tell me what the correct procedure is?

@prasadtalasila
Copy link
Contributor

@prasadtalasila I am having some problems with rebasing gitlab to distributed-demo, can you help me please?

I used the command git rebase origin/feature/distributed-demo within the gitlab branch. However, after manually merging the conflicting files, it merged the gitlab branch into the gitlab branch, while distributed-demo is not modified in any way. Could you please tell me what the correct procedure is?

feature/distributed-demo doesn't get modified with the rebase. It is only the gitlab branch that gets rebased. The resolved merge conflicts might be showing as merges. If there is still conflicts, we can take them up tomorrow. You can also look at this tutorial.

@VanessaScherma
Copy link
Contributor Author

@prasadtalasila I am having some problems with rebasing gitlab to distributed-demo, can you help me please?
I used the command git rebase origin/feature/distributed-demo within the gitlab branch. However, after manually merging the conflicting files, it merged the gitlab branch into the gitlab branch, while distributed-demo is not modified in any way. Could you please tell me what the correct procedure is?

feature/distributed-demo doesn't get modified with the rebase. It is only the gitlab branch that gets rebased. The resolved merge conflicts might be showing as merges. If there is still conflicts, we can take them up tomorrow. You can also look at this tutorial.

So the process would be: within the distributed-demo branch, do git rebase gitlab?

}
};

export const checkFirstPipelineStatus = async (
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 4 locations. Consider refactoring.

}
};

const handlePipelineCompletion = async (
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 4 locations. Consider refactoring.

);
};

const retryPipelineCheck = (
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 4 locations. Consider refactoring.

);
};

export const checkSecondPipelineStatus = async (
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 4 locations. Consider refactoring.

Copy link

codeclimate bot commented Sep 5, 2024

Code Climate has analyzed commit 94614ec and detected 8 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 4
Duplication 4

View more on Code Climate.

@VanessaScherma
Copy link
Contributor Author

@prasadtalasila

I am working on the required tests and the only tests left are for the file ExecutionFunctions.ts. However, I am experiencing a problem that I cannot solve.
When I run the tests, I get this error about the strip-ansi library:

`Jest encountered an unexpected token

Jest failed to parse a file. This happens e.g. when your code or its dependencies use non-standard JavaScript syntax, or when Jest is not configured to support such syntax.

Out of the box Jest supports Babel, which will be used to transform your files into valid JS based on your Babel configuration.

By default "node_modules" folder is ignored by transformers.

Here's what you can do:
 • If you are trying to use ECMAScript Modules, see https://jestjs.io/docs/ecmascript-modules for how to enable it.
 • If you are trying to use TypeScript, see https://jestjs.io/docs/getting-started#using-typescript
 • To have some of your "node_modules" files transformed, you can specify a custom "transformIgnorePatterns" in your config.
 • If you need a custom transformation specify a "transform" option in your config.
 • If you simply want to mock your non-JS modules (e.g. binary assets) you can stub them out with the "moduleNameMapper" config option.

You'll find more details and examples of these config options in the docs:
https://jestjs.io/docs/configuration
For information about custom transformations, see:
https://jestjs.io/docs/code-transformation

Details:

/home/vanessa/Documenti/DtaaS/DTaaS/client/node_modules/strip-ansi/index.js:1
({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,jest){import ansiRegex from 'ansi-regex';
                                                                                  ^^^^^^

SyntaxError: Cannot use import statement outside a module

   6 | import DigitalTwin from 'util/gitlabDigitalTwin';
   7 | import { GitlabInstance } from 'util/gitlab';
>  8 | import stripAnsi from 'strip-ansi';
     | ^
   9 | import { JobLog } from '../../components/asset/StartStopButton';
  10 |
  11 | export const handleButtonClick = (

  at Runtime.createScriptFromCode (node_modules/jest-config/node_modules/jest-runtime/build/index.js:1505:14)
  at Object.require (src/route/digitaltwins/ExecutionFunctions.ts:8:1)
  at Object.<anonymous> (test/unit/routes/digitaltwins/ExecutionFunctions.test.ts:1:1)`

I even tried running an isolated test to verify that the library is loaded correctly, but the test fails and always returns the library index file, pointing out how the import is wrong.
I've tried changing the import method, to use require instead of import, but I can't get the tests to work. I did not want to edit the ts.config file so as not to compromise the other liberias. How can I solve this problem?

Thank you.

@prasadtalasila
Copy link
Contributor

PR #903 includes the changes suggested here.

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.

3 participants