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

feat(RHINENG-7074): Update inventory fec config #2194

Merged
merged 10 commits into from
Jun 25, 2024

Conversation

adonispuente
Copy link
Contributor

@adonispuente adonispuente commented May 9, 2024

No description provided.

@adonispuente adonispuente requested a review from a team as a code owner May 9, 2024 19:57
@adonispuente adonispuente marked this pull request as draft May 9, 2024 20:19
@adonispuente adonispuente force-pushed the refactor branch 2 times, most recently from c79b3d1 to d06d7bf Compare May 13, 2024 21:29
@adonispuente adonispuente marked this pull request as ready for review May 13, 2024 21:29
@adonispuente adonispuente changed the title update components to pf5 feat(RHINENG-7074) : Update inventory fec config May 13, 2024
fec.config.js Outdated Show resolved Hide resolved
fec.config.js Outdated Show resolved Hide resolved
@bastilian
Copy link
Member

@adonispuente We should add .cache to the .gitignore and not commit the contents of this directory.

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
pr_check.sh Show resolved Hide resolved
@gkarat gkarat requested a review from a team May 16, 2024 09:46
.travis.yml Show resolved Hide resolved
fec.config.js Outdated Show resolved Hide resolved
@adonispuente adonispuente force-pushed the refactor branch 2 times, most recently from a3a6aaf to 159ec85 Compare May 17, 2024 14:33
@gkarat gkarat requested a review from a team May 30, 2024 09:11
@adonispuente adonispuente force-pushed the refactor branch 4 times, most recently from d4a2c3b to e2279ec Compare June 7, 2024 10:52
@codecov-commenter
Copy link

codecov-commenter commented Jun 7, 2024

Codecov Report

Attention: Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.

Project coverage is 57.69%. Comparing base (894b14d) to head (3f996b9).
Report is 2 commits behind head on master.

Files Patch % Lines
.../components/InventoryHostStaleness/BaseDropDown.js 87.50% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #2194       +/-   ##
===========================================
- Coverage   76.08%   57.69%   -18.39%     
===========================================
  Files         193      193               
  Lines        5038     5203      +165     
  Branches     1779     1902      +123     
===========================================
- Hits         3833     3002      -831     
- Misses       1205     2201      +996     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@adonispuente adonispuente force-pushed the refactor branch 2 times, most recently from 0487f92 to 7f235f9 Compare June 21, 2024 15:23
@gkarat
Copy link
Contributor

gkarat commented Jun 25, 2024

@adonispuente, I think we've got one merge conflict after the recent push to master. Can you please take a look?

Copy link
Contributor

@mkholjuraev mkholjuraev left a comment

Choose a reason for hiding this comment

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

LGTM! Conflicts seem to be minor. Thus approving. If you feel another eye is needed, feel free to ping

@adonispuente
Copy link
Contributor Author

/retest

1 similar comment
@Victoremepunto
Copy link

/retest

@aptmac
Copy link
Contributor

aptmac commented Jun 25, 2024

Hi! I've been keeping tabs on this PR while working on something similar for the insights-runtimes-frontend repo. Just a couple of things I've come across:

  1. I've noticed that the fec.config.js here is missing the custom proxy when running npm run start:mock, so the prism mock server isn't used.

  2. I've also noticed (and something I'm looking into at the moment) is that trying to specify a port number isn't working when running a LOCAL_APPS setup. npm run start -- --port=8004 for example doesn't take the 8004 into consideration, and for me at least it starts up on port 8002.

@adonispuente
Copy link
Contributor Author

@aptmac I took a look around some other applications and it seems that this is the case for the majority.
npm run start:mock isnt set up in the inventory repo itself, and the port issue is also in other apps as well. I think that has to be resolved in the package itself and then delivered to all the apps via package update or another means

@aptmac
Copy link
Contributor

aptmac commented Jun 25, 2024

@aptmac I took a look around some other applications and it seems that this is the case for the majority. npm run start:mock isnt set up in the inventory repo itself, [..]

It used to be in the dev.webpack.config.js see: here, FWIW I've added it into the runtimes-frontend repo the same way it was in the old webpack and it's been working, see: https://github.com/RedHatInsights/insights-runtimes-frontend/pull/16/files#diff-d0f06d7cd724a6add60ee1bac8183344dc66faae9dac3ee6ee920f5b7ac3a88eR18

@adonispuente
Copy link
Contributor Author

@aptmac thank you! Im not against it, but now that I see the other converted apps are ran the same way im thinking maybe we wanted to remove the mock option for some reason, or if there was just going to be a short period before the new set up can handle it 'directly'.
Nice fix though, im gonna ask around and see if theres a reason, but if not then I can add that in, or at least look into how to bake it into the package

@aptmac
Copy link
Contributor

aptmac commented Jun 25, 2024

Was just snooping around the frontend-components repo and found RedHatInsights/frontend-components#2007, looks like that might address the LOCAL_APPS in the future, just a heads up!

@adonispuente adonispuente merged commit 91da855 into RedHatInsights:master Jun 25, 2024
2 checks passed
adonispuente added a commit that referenced this pull request Jun 26, 2024
@adonispuente adonispuente changed the title feat(RHINENG-7074) : Update inventory fec config feat(RHINENG-7074): Update inventory fec config Jun 26, 2024
@ezr-ondrej
Copy link
Member

do I read it correctly image build for this have failed?

@mkholjuraev
Copy link
Contributor

do I read it correctly image build for this have failed?

Yes, it seems the image is not built. We will have a look why

LightOfHeaven1994 pushed a commit to LightOfHeaven1994/insights-inventory-frontend that referenced this pull request Jun 26, 2024
* feat(RHINENG-7047): Upgrade inventory FEC config
@@ -1,7 +1,7 @@
import { FormSpy, useFormApi } from '@data-driven-forms/react-form-renderer';
import { Button, Flex, Icon } from '@patternfly/react-core';
import ExclamationTriangleIcon from '@patternfly/react-icons/dist/js/icons/exclamation-triangle-icon';
import warningColor from '@patternfly/react-tokens/dist/esm/global_warning_color_100';
import { warningColor } from '@patternfly/react-tokens';
Copy link
Member

Choose a reason for hiding this comment

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

This should have been changed too, like the ones above. it is now raising an error when building:
Screenshot 2024-06-26 at 13 42 54

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.

8 participants