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

Feature request: Keep the configuration in a file #52

Closed
1 task done
PuneetGopinath opened this issue May 22, 2021 · 23 comments · Fixed by #53
Closed
1 task done

Feature request: Keep the configuration in a file #52

PuneetGopinath opened this issue May 22, 2021 · 23 comments · Fixed by #53
Assignees
Labels
major Will be added to next major release Priority: medium Medium priority for this issue/pr. Type: breaking This change will break previous versions

Comments

@PuneetGopinath
Copy link
Member

Please check these things before submitting your issue:

  • I searched for duplicate or closed feature request here

Is your feature request related to a problem? Please describe.
N/A.

Describe the solution you'd like
I think you should keep the configuration in a separate file (maybe a file like .github/recent-activity-config.yml) as there are too many inputs and only add config_file as a option in github action inputs which will tell path to the configuration file. I will assign this to my self if you agree with this request and plz help me in the process of doing it.

Describe alternatives you've considered
N/A.

Additional context
Probaly I will use js-yaml as a dependency to do parse the yaml config.

@PuneetGopinath PuneetGopinath added major Will be added to next major release Type: breaking This change will break previous versions Priority: medium Medium priority for this issue/pr. labels May 22, 2021
@PuneetGopinath PuneetGopinath self-assigned this May 22, 2021
@abhijoshi2k
Copy link
Member

Yes. We can have a recent-activity.config.json named file.

@Andre601
Copy link
Member

Agree. Moving the settings to a configuration file seems like a good idea.

I personally have the following file structure in mind:

settings:
  username: 'Readme-Workflows'
  commit_message: '⚡ Update README with the recent activity'
  max_lines: 5
  readme_file: './README.md'
  disabled_events: # Maybe we can now have an actual list?
  - 'comment'
  url_text: '{REPO}{ID}'
  date:
    timezone: '0:00' # Would also allow defining timezone names if #49 is worked on
    text: 'Last updated: {DATE}'
    date_format: 'dddd, mmmmm dS, yyyy, h:MM:ss TT'
messages:
  comments: '💬 Commented on {ID} in {REPO}'
  issue_open: '❗️ Opened issue {ID} in {REPO}'
  issue_close: '✔ Closed issue {ID} in {REPO}'
  pr_open: '💪 Opened PR {ID} in {REPO}'
  pr_close: '❌ Closed PR {ID} in {REPO}'
  pr_merge: '🎉 Merged PR {ID} in {REPO}'
  repo_create: '📔 Created new repository {REPO}'
  repo_fork: '🔱 Forked {FORK} from {REPO}'
  repo_watch: '👀 Watching {REPO}'
  wiki_create: '📖 Created new wiki page {WIKI} in {REPO}'

@abhijoshi2k
Copy link
Member

@Andre601 that'd be fine too. However, I think we should keep these parameters in uppercase.
Also, @PuneetGopinath we should work on it after #50 is merged. Give me some time for it.

@PuneetGopinath
Copy link
Member Author

PuneetGopinath commented May 22, 2021

Yes can have a actually list for DISABLE_EVENTS.
And as said by @abhijoshi2k We have to keep it uppercase

@Andre601
Copy link
Member

@Andre601 that'd be fine too. However, I think we should keep these parameters in uppercase.
Also, @PuneetGopinath we should work on it after #50 is merged. Give me some time for it.

I dislike uppercase names in YAML names, because it just looks shit.
In env variables does it make sense as that is the common practice there, but YAML configurations usually have lowercase or lower-camelcase used as UPPERCASE MAKES IT LOOK MORE ANNOYING, OR DO YOU LIKE THIS TEXT HERE?

@PuneetGopinath
Copy link
Member Author

PuneetGopinath commented May 22, 2021

Annoying, ok, then first replace underscore (_) with hiphen (-)

@abhijoshi2k
Copy link
Member

Annoying, ok, then first replace underscore (_) with hifen (-)

We cannot do that. At the end, we have to convert it into object and it does not accept hiphen (-).

@Andre601
Copy link
Member

Andre601 commented May 22, 2021

It's "hyphen" to my knowledge...

@PuneetGopinath
Copy link
Member Author

It's "hyphen" to my knowledge...

It was a typo.

@Andre601
Copy link
Member

Annoying, ok, then first replace underscore (_) with hifen (-)

We cannot do that. At the end, we have to convert it into object and it does not accept hiphen (-).

The goal of the file would be to have easier configuration options. Env vars are limited and only converting those settings into a a env would be extremely pointless imo.
If we already make a separate file then we should use their benefits and not just have a setting -> env way.

@abhijoshi2k
Copy link
Member

We will have to use javascript spread operator to overwrite the default config.
@PuneetGopinath can you create a javascript module which will read the yaml and export an object with all the parameters of yaml?

@PuneetGopinath
Copy link
Member Author

The goal of the file would be to have easier configuration options. Env vars are limited and only converting those settings into a a env would be extremely pointless imo.
If we already make a separate file then we should use their benefits and not just have a setting -> env way.

I think you didn't understand what @abhijoshi2k said. He said that when parsing yaml file it will convert it into a object and in a object hyphen are not allowed in Node.js

@PuneetGopinath
Copy link
Member Author

We will have to use javascript spread operator to overwrite the default config.
@PuneetGopinath can you create a javascript module which will read the yaml and export an object with all the parameters of yaml?

Will do it.

@PuneetGopinath
Copy link
Member Author

PuneetGopinath commented May 22, 2021

@abhijoshi2k I am going to have lunch now, today it became little late, can I do it afterwards?

@abhijoshi2k
Copy link
Member

@abhijoshi2k I am going to have lunch now, today it because little late, can I do it afterwards?

Yeah take your time

@PuneetGopinath
Copy link
Member Author

@abhijoshi2k Should I edit the config.js directly? Or create a new function parseYaml?

const parseYaml = (file) => {
  return config = yaml.load(fs.readFileSync(file, "utf8"));
};

@abhijoshi2k
Copy link
Member

I suggest create new function that will return an object. So we can call the function from config.js and then replace the default values with spread operator.

@abhijoshi2k
Copy link
Member

We shall first populate an object variable with default statements and then update it with custom statements (custom statements for whichever events provided). Make sure the arguments are named the same everywhere, else it won't overwrite.
If needed, change default arguments name.

@PuneetGopinath
Copy link
Member Author

@abhijoshi2k Can you update package-lock.json by running npm install?

@PuneetGopinath
Copy link
Member Author

@abhijoshi2k I think we can't get GH_USERNAME using ${{ github.repository_owner }}, So we should keep that as a GitHub action input, right?

@abhijoshi2k
Copy link
Member

@abhijoshi2k I think we can't get GH_USERNAME using ${{ github.repository_owner }}, So we should keep that as a GitHub action input, right?

We are getting it with ${{ github.repository_owner }} already. Though you can add it in the separate YAML you are making. It will just replace.

@PuneetGopinath
Copy link
Member Author

I asked because I don't know whether that var is defined in our files, it maybe defined by dependencies like actions/core.

@abhijoshi2k
Copy link
Member

I asked because I don't know whether that var is defined in our files, it maybe defined by dependencies like actions/core.

It is defined by github when action starts and given as input.
We get it as: const GH_USERNAME = core.getInput("GH_USERNAME");

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major Will be added to next major release Priority: medium Medium priority for this issue/pr. Type: breaking This change will break previous versions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants