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

nautilus: mgr/dashboard: switch ng2-toastr to ngx-toastr #29050

Merged
merged 1 commit into from Jul 22, 2019

Conversation

@epuertat
Copy link
Contributor

commented Jul 15, 2019

NPM package ng2-toastr is currently downloaded from a Github repo (https://github.com/zzakir/ng2-toastr). This makes it imossible to use npm cache, as non-NPMJS registry packages are always fetched from the original site. This has been fixed in master during the Boostrap upgrade #25188.

captured

As this means git is no longer needed during dashboard build, we could also revert #24520.


Fixes: https://tracker.ceph.com/issues/40768
Signed-off-by: Tiago Melo tmelo@suse.com
(cherry picked from commit 67f5e5b)
Signed-off-by: Ernesto Puerta epuertat@redhat.com

Required manual (trivial) fixing of the following files:
Conflicts:
- src/pybind/mgr/dashboard/frontend/package.json
- src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/prometheus/alert-list/alert-list.component.spec.ts
- src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/prometheus/silence-form/silence-form.component.spec.ts
- src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/prometheus/silence-list/silence-list.component.spec.ts
- src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/pool-list/pool-list.component.spec.ts
- src/pybind/mgr/dashboard/frontend/src/app/shared/services/prometheus-notification.service.spec.ts

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug

@epuertat epuertat added this to the nautilus milestone Jul 15, 2019

@epuertat epuertat self-assigned this Jul 15, 2019

@epuertat epuertat changed the title mgr/dashboard: switch ng2-toastr to ngx-toastr nautilus: mgr/dashboard: switch ng2-toastr to ngx-toastr Jul 16, 2019

@epuertat epuertat requested a review from tspmelo Jul 16, 2019

@epuertat epuertat marked this pull request as ready for review Jul 16, 2019

@epuertat epuertat requested review from Devp00l and votdev Jul 16, 2019

@tspmelo
Copy link
Contributor

left a comment

lgtm

@tspmelo
Copy link
Contributor

left a comment

Just notice that the commit msg is not correct.
For some reason you are doing a cherry-pick of a cherry-pick.
This should be a direct cherry-pick of the original commit.

@votdev
votdev approved these changes Jul 17, 2019
mgr/dashboard: Switch ng2-toastr for ngx-toastr
Fixes: https://tracker.ceph.com/issues/40768
Signed-off-by: Tiago Melo <tmelo@suse.com>
(cherry picked from commit 67f5e5b)
Signed-off-by: Ernesto Puerta <epuertat@redhat.com>

Required manual (trivial) fixing of the following files:
Conflicts:
      src/pybind/mgr/dashboard/frontend/package.json
      src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/prometheus/alert-list/alert-list.component.spec.ts
      src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/prometheus/silence-form/silence-form.component.spec.ts
      src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/prometheus/silence-list/silence-list.component.spec.ts
      src/pybind/mgr/dashboard/frontend/src/app/ceph/pool/pool-list/pool-list.component.spec.ts
      src/pybind/mgr/dashboard/frontend/src/app/shared/services/prometheus-notification.service.spec.ts

@epuertat epuertat force-pushed the rhcs-dashboard:fix-40768-nautilus branch from 14b5492 to aad32e0 Jul 17, 2019

@epuertat epuertat requested review from tspmelo and removed request for Devp00l Jul 18, 2019

@epuertat

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

Just notice that the commit msg is not correct.
For some reason you are doing a cherry-pick of a cherry-pick.
This should be a direct cherry-pick of the original commit.

@tspmelo done! I just did an intermediary cherry-pick in master (pre-Bootstrap PR) before bringing this to Nautilus. It applied cleanly to Nautilus, so it's a 1:1 copy of pre-BS4 master cherry-pick. Nevertheless, apart from the line "cherry picked from" git does nothing else for tracking commits between cherry-picks. Just modified that line.

@tspmelo

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

As this means git is no longer needed during dashboard build, we could also revert #24520.

I hope we don't need to add anymore packages via git, but it could happen again.
So I would suggest to not revert that.

@epuertat epuertat requested review from smithfarm, yuriw, alfonsomthd and a2batic Jul 18, 2019

@epuertat

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

As this means git is no longer needed during dashboard build, we could also revert #24520.

I hope we don't need to add anymore packages via git, but it could happen again.
So I would suggest to not revert that.

I think we should revert it (regardless this change) because it's not correct: install-deps.sh is meant to install deps, not to fail if some dep is not there, especially when in fact this script installs git in RPM, DEB, FreeBSD and Alpine environments. So if git is somehow still missing, the expected action is getting it installed, not failing.

Besides, I like how liberal we are with NPM packages compared to Python, but perhaps downloading snapshots from "unofficial" Github forks is going too far (besides it breaks npm cache functionality).

@yuriw

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

@smithfarm

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2019

@epuertat Nit regarding the commit message. We have an established "best practice" of leaving the original commit message intact when cherry-picking. Please limit editing of cherry-pick commit messages to the area below the "(cherry picked from ...)" line.

In this case, you added a line "Fixes: https://tracker.ceph.com/issues/40768" to the original commit message . . . this line does not relate to the original commit, but rather to the backport, so it should have been added below "(cherry picked from. . .)" and not above.

Something for next time - this one is already undergoing integration testing, so let's leave it as-is.

(This suggestion may or may not seem pedantic. FWIW, I believe there is value in having everyone adhere to a consistent format when writing backport commit messages. And all that is really needed is git cherry-pick -x and a description of any manually resolved conflicts. Additional information can be added at the backporter's option, but is not required.)

@epuertat

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2019

(This suggestion may or may not seem pedantic. FWIW, I believe there is value in having everyone adhere to a consistent format when writing backport commit messages. And all that is really needed is git

Thanks @smithfarm ! Yeah, I (mostly) always follow this practice since you told me (I used git cp, which is my alias for cherry-pick -xsS), but in this specific case I chose to 'modify' it a bit because the original commit didn't have Fixes field and had missing changes (2 missing files that were amended in an unrelated commit of the same PR: 07f2805), and also there were changes to filenames/locations that made the cherry-picking/backporting not straightforward (not a nightmare but requires some find/grep magic to relocate delta chunks).

Nevertheless, I'll strictly follow these rules next time. Thanks for letting me know!

@smithfarm

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2019

@epuertat Thanks. I wouldn't have said a word if you had put the additional information under the original commit message, instead of inside it :-) Happy hacking!

@yuriw
yuriw approved these changes Jul 22, 2019

@yuriw yuriw merged commit e53e4d6 into ceph:nautilus Jul 22, 2019

5 checks passed

Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
ceph dashboard tests ceph dashboard tests succeeded
Details
make check make check succeeded
Details

@epuertat epuertat deleted the rhcs-dashboard:fix-40768-nautilus branch Aug 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.