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

frontend: change history link in data job #1884

Merged

Conversation

gorankokin
Copy link
Contributor

@gorankokin gorankokin commented Apr 12, 2023

1st commit:
frontend: Propagate features from @taurus/shared

Propagate 3 features from @taurus/shared

  • dynamic-components
  • confirmation
    • confirmation feature is extended to support message
      component in addition of supported message text
  • url-opener

There is no expected regression since adding only
new features that are not touching anything of existing
codebase.
Features are almost 100% covered with unit,
functional tests.
Tested with generating local build, linking built artefacts
back to @taurus/shared and then in other libraries
and manual validated that everything works correctly,
prettier for formatting executed for the new files and also
ESLint executed successfully and there are no error issues.

2nd commit:
frontend: Implement changed history for Job

Implement feature for Data Job changed history link that is
instructed with feature flag whether to show or hide, and
on which page to show.
Confirmation title and message are provided as dependency
injection to the library config.
Tested with generating local build, linking built artefacts
back to @taurus/shared and then in other libraries
and manual validated that everything works correctly.
Executed prettier for formatting new files and also
successfully executed ESLint and there are no error issues.

@gorankokin gorankokin force-pushed the person/gorankokin/add-change-history-link-in-data-job branch from 661d546 to d696306 Compare April 12, 2023 15:57
@gorankokin gorankokin added the area/frontend Related to changes in the folder projects/frontend for details about ui please see readme label Apr 12, 2023
@gorankokin gorankokin force-pushed the person/gorankokin/add-change-history-link-in-data-job branch from d696306 to 331c42c Compare April 12, 2023 16:42
@antoniivanov
Copy link
Collaborator

I recently started using stacked PRs. (this article is great intro- https://benjamincongdon.me/blog/2022/07/17/In-Praise-of-Stacked-PRs/ )

I use git machete to manage my branches and I've found that it's much easier to juggle multiple different changes.

That being said, it's ok to submit both in on PR now. Please make sure to click "Rebase and merge" when merging as the default is "squash")

Please fill in the testing done section.

Copy link
Collaborator

@antoniivanov antoniivanov left a comment

Choose a reason for hiding this comment

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

Personally I'd have made it more configurable/extensible instead of feature fflag.

Like have ExteranlUrlBasedSectionService or something along those lines and somewhere we can configigure

title="Change History"
urlTemplate="http://xxx/{jobName}" 
....

If we need one such external URL, we may nee more.
I can imagine one for JupyterHub integration potentially.

But I am ok with the feature flag as well.

I have reviewed only the second commit. The only thing I can note I find the lack of tests a bit disturbing.
And also I reviewed it mostly from functional perspective. It would be need to be reviewed from someone who knows angular/typescript and generally UI development.

I assume there are not functional changes in the first commit from the original source.

Please do add screenshots .

@gorankokin
Copy link
Contributor Author

gorankokin commented Apr 13, 2023

I recently started using stacked PRs. (this article is great intro- https://benjamincongdon.me/blog/2022/07/17/In-Praise-of-Stacked-PRs/ )

I use git machete to manage my branches and I've found that it's much easier to juggle multiple different changes.

That being said, it's ok to submit both in on PR now. Please make sure to click "Rebase and merge" when merging as the default is "squash")

Please fill in the testing done section.

I didn't use stacked PR because of the needs to switch branches and make rebases on every change of the root PR which consumes additional time, but I agree it's easier to track changes and review them.
Also I want to add that to make stacked PR additional architectural/thinking time is needed in advance before actual implementation is started in order to make commits self-contained and separated in different PR (interfaces for every aspect of the feature should be made in advance in order no to waste time switching branches and committing then switching new branch again, rebasing, etc...) that would increase development cost for new feature, if changes are needed in different functional areas in the library.

Currently we already have long path to propagate some change from @versatiledatakit/shared to SC, it should go through VDK CI/CD, then through @taurus/shared CI/CD, then through @taurus/metadata CI/CD, and at the end to adopt the new versions in SC and deploy through SC CI/CD.

In future I suppose there won't be a need for such huge PR, because almost everything is propagated from @taurus/shared to @versatiledatakit/shared and I think new shared features would be developed only in @versatiledatakit/shared, to avoid cases like this where we left shared features not used in VDK UI to live in @taurus/shared but on the first feature we needed to propagate them too.

@tozka
I have written section for testing in every commit message how it's been tested, it's in the description of this PR.

Tested with generating local build, linking built artefacts
back to @taurus/shared and then in other libraries
and manual validated that everything works correctly,
prettier for formatting executed for the new files and also
ESLint executed successfully and there are no error issues.

@gorankokin
Copy link
Contributor Author

gorankokin commented Apr 13, 2023

Personally I'd have made it more configurable/extensible instead of feature fflag.

Like have ExteranlUrlBasedSectionService or something along those lines and somewhere we can configigure

title="Change History"
urlTemplate="http://xxx/{jobName}" 
....

If we need one such external URL, we may nee more. I can imagine one for JupyterHub integration potentially.

But I am ok with the feature flag as well.

I have reviewed only the second commit. The only thing I can note I find the lack of tests a bit disturbing. And also I reviewed it mostly from functional perspective. It would be need to be reviewed from someone who knows angular/typescript and generally UI development.

I assume there are not functional changes in the first commit from the original source.

Please do add screenshots .

  • @tozka could you explain me a little bit deeper, what do you mean by ExternalUrlSectionService for current URLs and for future one? What would be the context of the service, where should it live?
    For this particular case where would be the knowledge for the title and urlTemplate?
  1. Would it be in the UI Application code, or
  2. Would it be in some environment variables in the deployment environment, or
  3. Would it be somewhere in static file that would be loaded before Application is bootstraped?
  • only the second commit is actual the change implementation, the first one is features propagation from @taurus/shared which are unit tests covered 95%+

  • tests are not added because in Taurus project we have agreement to not write unit tests for Components and that they would be tested with e2e functional tests, but for this particular feature, tests couldn't be written in this repository, because this features is not showed in context of VDK, it's SC specific.

  • I didn't add screenshots, because screenshots are actually from SC console which is internal VMware project and I don't know if it's good to expose internal project look and feel in Github, I've don't know what are the actual recommendation for this thing. Screenshots could be seen in control-plane team UX channel in slack, where I think you are member too

@gorankokin gorankokin force-pushed the person/gorankokin/add-change-history-link-in-data-job branch 3 times, most recently from 88ccd31 to 6912080 Compare April 13, 2023 17:40
@gorankokin gorankokin force-pushed the person/gorankokin/add-change-history-link-in-data-job branch 3 times, most recently from 43116af to 0a3d938 Compare April 25, 2023 19:41
Propagate 3 features from @taurus/shared
 - dynamic-components
 - confirmation
   - confirmation feature is extended to support message
     component in addition of supported message text
 - url-opener

There is no expected regression since adding only
new features that are not touching anything of existing
codebase.
Features are almost 100% covered with unit,
functional tests.
Tested with generating local build, linking built artefacts
back to @taurus/shared and then in other libraries
and manual validated that everything works correctly,
prettier for formatting executed for the new files and also
ESLint executed successfully and there are no error issues.

Signed-off-by: gorankokin <gorankokin@gmail.com>
Implement feature for Data Job changed history link that is
instructed with feature flag whether to show or hide, and
on which page to show.
Confirmation title and message are provided as dependency
injection to the library config.
Tested with generating local build, linking built artefacts
back to @taurus/shared and then in other libraries
and manual validated that everything works correctly.
Executed prettier for formatting new files and also
successfully executed ESLint and there are no error issues.

Signed-off-by: gorankokin <gorankokin@gmail.com>
@gorankokin gorankokin force-pushed the person/gorankokin/add-change-history-link-in-data-job branch from 0a3d938 to b1b00f5 Compare April 25, 2023 22:22
@gorankokin gorankokin enabled auto-merge (rebase) April 25, 2023 22:23
@gorankokin gorankokin merged commit 906b639 into main Apr 25, 2023
6 checks passed
@gorankokin gorankokin deleted the person/gorankokin/add-change-history-link-in-data-job branch April 25, 2023 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend Related to changes in the folder projects/frontend for details about ui please see readme cla-not-required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants