-
Notifications
You must be signed in to change notification settings - Fork 1.7k
AMBARI-26175 Register version capabilities in Ambari Admin #3971
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
base: frontend-refactor
Are you sure you want to change the base?
AMBARI-26175 Register version capabilities in Ambari Admin #3971
Conversation
@zRains Can you please help with the review? |
@vanshuhassija Sorry about this, I don’t have much spare time right now. I’ll get to it by the weekend at the latest. |
0fd385c
to
a8330c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems your code contains many any
types, which weakens the functionality of TypeScript and makes troubleshooting more difficult. If the type is currently unclear, please try to use unknown
instead.
type ModalProps = { | ||
isOpen: boolean | ||
onClose: () => void | ||
onReadVersion: Function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type is vague, please specify.
type="file" | ||
className="py-1" | ||
onChange={() => { | ||
//@ts-ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please try to avoid using this identifier.
<Modal.Header closeButton> | ||
<Modal.Title>Public Repository Option Disabled</Modal.Title> | ||
</Modal.Header> | ||
<Modal.Body style={{ fontSize: 12 }}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please try to avoid using inline styles; you can use Tailwind instead.
/* eslint-disable no-useless-escape */ | ||
/* eslint-disable no-unsafe-optional-chaining */ | ||
/* eslint-disable @typescript-eslint/ban-ts-comment */ | ||
/* eslint-disable @typescript-eslint/no-explicit-any */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please try to avoid using these identifiers; if certain factors necessitate their use, please specify the reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix errors in test files Register.test.tsx as mentioned in the CI pipeline
eg
[INFO] src/tests/StackVersions/Register.test.tsx(47,17): error TS2769: No overload matches this call. [INFO] Overload 1 of 2, '(props: RouterProps): Router', gave the following error. [INFO] Type 'MemoryHistory' is missing the following properties from type 'History<unknown>': length, goBack, goForward [INFO] Overload 2 of 2, '(props: RouterProps, context: any): Router', gave the following error.
Here's the English translation: The RPM package names compiled by Ambari for Bigtop and their component names during yum installation are inconsistent with the official Bigtop binary RPM names. This situation forces users to only use Bigtop RPM packages compiled by Ambari. For example, the official Bigtop package name is 'hadoop-3.3.6-1.el7.x86_64.rpm', while the Ambari-compiled Bigtop RPM package name is 'hadoop_3_3_0-3.3.6-1.el8.x86_64.rpm'. |
Replace stack Fix typescript changes
a8330c2
to
37b62ed
Compare
@zRains I have addressed some of the typescript issues. I intend to address more in future PRs before finalising this project. Merging this PR can also open the feature to community to make some changes to the implemented feature. |
What changes were proposed in this pull request?
Add capability to register Stack version
How was this patch tested?
Manually, test cases included