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

Create a per-dashboard config #235

Closed
armenzg opened this issue Oct 17, 2019 · 16 comments · Fixed by #271
Closed

Create a per-dashboard config #235

armenzg opened this issue Oct 17, 2019 · 16 comments · Fixed by #271

Comments

@armenzg
Copy link
Collaborator

@armenzg armenzg commented Oct 17, 2019

Currently I have two different branches for two different projects:
https://firefox-performance-dashboard.netlify.com
https://awsy.netlify.com

I would like to either join both repos back or at least have different config files so I can merge changes from master into the awsy branch.

The file that conflicts between the two is src/config.js.

If we could have src/dashboards/awfy.js and src/dashboards/awsy.js it would be great!

@divyanshu-rawat

This comment has been minimized.

Copy link
Collaborator

@divyanshu-rawat divyanshu-rawat commented Dec 6, 2019

Hey, @armenzg I am interested in this issue, let me know what it is about? and what you want? :)

@armenzg

This comment has been minimized.

Copy link
Collaborator Author

@armenzg armenzg commented Dec 9, 2019

Hi @divyanshu-rawat thanks for your interest.

There are two branches on this repo: master and awsy. They each configure a different dashboard (see 1st comment). The configuration files for each dashboard is in their own branch:

If would be ideal to be able to start one dashboard or the other one from the same branch. In other words, only having the master branch. Perhaps we can do so by one of the following ideas (I don't know which one is possible):

  • yarn start path/to/config.js
  • yarn start name_of_dashboard (internally having an object that points to one of the two configs).

Let me know if this helps. Thanks!

This was referenced Dec 9, 2019
@divyanshu-rawat

This comment has been minimized.

Copy link
Collaborator

@divyanshu-rawat divyanshu-rawat commented Dec 10, 2019

cool @armenzg I will read through what you have mentioned in the comment and then let you know. 🙌

@divyanshu-rawat

This comment has been minimized.

Copy link
Collaborator

@divyanshu-rawat divyanshu-rawat commented Dec 12, 2019

If we could have src/dashboards/awfy.js and src/dashboards/awsy.js it would be great!

Here in the last line what do you mean? it is the same file you are referring to. @armenzg

@divyanshu-rawat

This comment has been minimized.

Copy link
Collaborator

@divyanshu-rawat divyanshu-rawat commented Dec 12, 2019

Hi @divyanshu-rawat thanks for your interest.

There are two branches on this repo: master and awsy. They each configure a different dashboard (see 1st comment). The configuration files for each dashboard is in their own branch:

If would be ideal to be able to start one dashboard or the other one from the same branch. In other words, only having the master branch. Perhaps we can do so by one of the following ideas (I don't know which one is possible):

  • yarn start path/to/config.js
  • yarn start name_of_dashboard (internally having an object that points to one of the two configs).

Let me know if this helps. Thanks!

If we want to start both of them from the same branch let's say master, then I think I have to migrate the config files of both the projects to master.

what should I name the config file of master branch for awsy branch I can name it awsy.js , currently I plan to name master config as master.js.

Something like src/config/awsy.js and src/config/master.js what do you think?,

@armenzg

This comment has been minimized.

Copy link
Collaborator Author

@armenzg armenzg commented Dec 12, 2019

If we could have src/dashboards/awfy.js and src/dashboards/awsy.js it would be great!

Here in the last line what do you mean? it is the same file you are referring to. @armenzg

The names are very close, however, they're actually two differently named configs (to be created):

  • Config from master --> src/config/awfy.js (Notice the 'f' instead of an 's')
  • Config from awsy --> src/config/awsy.js (Notice the 's' instead of an 'f')

If we want to start both of them from the same branch let's say master, then I think I have to migrate the config files of both the projects to master.

You got it.

Something like src/config/awsy.js and src/config/master.js what do you think?,

Correct the 2nd name: master.js --> awfy.js

Thanks for looking into this! 👍

@divyanshu-rawat

This comment has been minimized.

Copy link
Collaborator

@divyanshu-rawat divyanshu-rawat commented Dec 13, 2019

@armenzg I spent much time on this and managed to find a solution for the same, but later one solution was not working for awsy.js and realised that structure of config object is different in different branches what I mean is if you search for term dayRange in the config of master and in the config of awsy, then in the config of master you will 2 occurrences of it, and in the config of awsy you will find one as shown in attached snapshots.

AWSY

Screenshot 2019-12-13 at 02 12 28

AWFY

Screenshot 2019-12-13 at 02 13 13

Which implies that due to difference in object structure the code written in 2 different branches is different at some places, for example.

Here is the example -

This is Pickers component in master

So, the master one Picker component looks like this.

    <Picker
      key="Time range"
      identifier="numDays"
      topLabel="Time range"
      onSelection={onChange}
      selectedValue={dayRange}
      options={CONFIG.dayRange.map((numDays) => ({
        value: numDays,
        label: generateLastDaysLabel(numDays),
      }))}
    />

Have a look at options property, that points to CONFIG.dayRange in config in master, but CONFIG.dayRange doesn't exist in awsy config, so Picker component throws an error if you load the config file for awsy in master branch.

And This is the pickers component in awsy

@divyanshu-rawat

This comment has been minimized.

Copy link
Collaborator

@divyanshu-rawat divyanshu-rawat commented Dec 13, 2019

One proposed solution is to add dayRange: [1, 2, 7, 14, 30, 60, 90, 365], in awsy as well, it will solve the problem and my solution to this issue will work. what do you say?

I have sent the PR, you can try running the app in localhost 😅

@armenzg

This comment has been minimized.

Copy link
Collaborator Author

@armenzg armenzg commented Dec 13, 2019

Yes, that's a fine idea.
I will look at the PR on Monday since I'm off today. Thanks for your help!

@divyanshu-rawat

This comment has been minimized.

Copy link
Collaborator

@divyanshu-rawat divyanshu-rawat commented Dec 13, 2019

cool @armenzg 👍

@armenzg armenzg closed this in #271 Jan 6, 2020
armenzg added a commit that referenced this issue Jan 6, 2020
* Support multiple dashboards
* Add DASHBOARD env to neutrino.
* Include awsy configuration file from [awsy branch](https://github.com/mozilla-frontend-infra/firefox-performance-dashboard/blob/awsy/src/config.js)


Co-authored-by: Divyanshu Rawat <divyanshu.r46956@gmail.com>
@armenzg

This comment has been minimized.

Copy link
Collaborator Author

@armenzg armenzg commented Jan 6, 2020

We can now deploy both dashboard from the same code.
yay!
https://awsy.netlify.com/win10/overview?numDays=60

@armenzg

This comment has been minimized.

Copy link
Collaborator Author

@armenzg armenzg commented Jan 6, 2020

I added DASHBOARD=awsy as an env variable and I pointed to master as the branch.
For now, I will keep the old branch.

@armenzg

This comment has been minimized.

Copy link
Collaborator Author

@armenzg armenzg commented Jan 6, 2020

Hi,
I see an email notification asking me if I can see the contribution.
Is this it?
image

@divyanshu-rawat

This comment has been minimized.

Copy link
Collaborator

@divyanshu-rawat divyanshu-rawat commented Jan 6, 2020

@armenzg I removed the comment thinking it doesn"t matter, but I was curious though that this contribution was not reflected at all, those 2 commits in the picture points to 2 different issues.

@armenzg

This comment has been minimized.

Copy link
Collaborator Author

@armenzg armenzg commented Jan 6, 2020

I have ammended the commit and force pushed it. You might need to force pull if you had already updated to master.
image

@divyanshu-rawat

This comment has been minimized.

Copy link
Collaborator

@divyanshu-rawat divyanshu-rawat commented Jan 6, 2020

Thanks @armenzg :) I wiil create new fork then to avoid issues ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants
You can’t perform that action at this time.