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

ACMS-1989: Update admin theme in Acquia CMS to Gin. #1649

Merged
merged 5 commits into from
Oct 19, 2023

Conversation

rajeshreeputra
Copy link
Contributor

@rajeshreeputra rajeshreeputra commented Oct 11, 2023

Motivation
Fixes #ACMS-1989

Proposed changes
This PR is created to check if there any test failure with Gin theme.

Alternatives considered
This can be closed without any further action once the issue identification is completed, or it can be used, and further changes can be made here directly.

Testing steps
NA

Merge requirements

  • Major change, Minor change, Bug, Enhancement, and/or Chore label applied
  • Manual testing by a reviewer

@rajeshreeputra rajeshreeputra force-pushed the ACMS-1989 branch 3 times, most recently from d20b107 to f0a062e Compare October 13, 2023 07:51
Copy link
Contributor

@apathak18 apathak18 left a comment

Choose a reason for hiding this comment

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

Added a minor suggestion rest everything looks good

@rajeshreeputra rajeshreeputra force-pushed the ACMS-1989 branch 2 times, most recently from 741c60f to 26e4f95 Compare October 16, 2023 08:48
Copy link
Collaborator

@chandan-singh7929 chandan-singh7929 left a comment

Choose a reason for hiding this comment

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

@rajeshreeputra - I have requested some changes, please take a look.

@rajeshreeputra rajeshreeputra force-pushed the ACMS-1989 branch 2 times, most recently from 882df23 to 9cbb267 Compare October 16, 2023 11:14
@rajeshreeputra rajeshreeputra changed the base branch from develop to feature/admin-gin-theme October 16, 2023 11:39
@rajeshreeputra rajeshreeputra force-pushed the ACMS-1989 branch 3 times, most recently from ec88a25 to 980270e Compare October 17, 2023 11:36
Copy link
Collaborator

@chandan-singh7929 chandan-singh7929 left a comment

Choose a reason for hiding this comment

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

@rajeshreeputra - I can see some changes which are not related to work of this ticket, like refactoring, I would suggest to do this in a separate ticket and lets add only changes which are related to this ticket.

Also don't include patch for the work that is not merged, rather provide steps to pull that in during verification.

@vishalkhode1 @vipin-mittal-acquia - what's your though on this?

@rajeshreeputra
Copy link
Contributor Author

rajeshreeputra commented Oct 19, 2023

I can see some changes which are not related to work of this ticket, like refactoring, I would suggest to do this in a separate ticket and lets add only changes which are related to this ticket.

Removed the changes not related to work of this ticket.

Also don't include patch for the work that is not merged, rather provide steps to pull that in during verification.

The patch has been added to the root composer.json file. In our CI environment, the site is installed using the starterkit, and it's important to note that there have been changes related to the Gin theme, although these changes have not been released yet. This means that our CI process will encounter failures because the starterkit version 1.1.5 now requires Acquia Claro to be installed in order to set up the site and run tests. However, in the pull request, Acquia Claro has been removed, causing the CI failures. Hence it is required to be added in root composer.json file.

@chandan-singh7929
Copy link
Collaborator

The patch has been added to the root composer.json file. In our CI environment, the site is installed using the starterkit, and it's important to note that there have been changes related to the Gin theme, although these changes have not been released yet. This means that our CI process will encounter failures because the starterkit version 1.1.5 now requires Acquia Claro to be installed in order to set up the site and run tests. However, in the pull request, Acquia Claro has been removed, causing the CI failures. Hence it is required to be added in root composer.json file.

ok, then lets create a ticket to remove this patch when we do the release for this changes.

@chandan-singh7929 chandan-singh7929 merged commit 72f3cb4 into feature/admin-gin-theme Oct 19, 2023
@chandan-singh7929 chandan-singh7929 deleted the ACMS-1989 branch October 19, 2023 09:09
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.

4 participants